iter-43: Pre-release DX fixes (eval --file, --full-page, computed, summary, --wait-idle)#45
Merged
iter-43: Pre-release DX fixes (eval --file, --full-page, computed, summary, --wait-idle)#45
Conversation
…ommand (iter-43) Addresses friction captured in dogfooding session: - eval now accepts --file / --stdin to sidestep shell quoting issues - screenshot --full-page / --viewport-height captures beyond the first fold - new computed command wraps getComputedStyle for CSS debugging - console output now includes a summary field (total, matched, shown, by_level) - reload --wait-idle blocks until network quiescence, replacing sleep hacks - navigate long_about clarifies URL is positional (no --url flag) - backlog entries for wait --console-includes and screenshot --annotate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (ff-rdp)
participant RDP as RDP Server
participant Firefox as Firefox Tab
CLI->>RDP: connect & authenticate
CLI->>RDP: getTarget()
RDP-->>CLI: target actor
CLI->>Firefox: eval computed-styles JS (selector + --prop/--all)
Firefox->>Firefox: query matching elements and compute styles
Firefox-->>RDP: evaluation result (JSON sentinel + payload)
RDP-->>CLI: result payload
CLI->>CLI: strip sentinel & parse JSON
CLI->>CLI: format output envelope
CLI-->>User: JSON output
sequenceDiagram
participant CLI as CLI (ff-rdp)
participant FS as File System
participant RDP as RDP Server
participant Firefox as Firefox Tab
CLI->>CLI: parse args (--file mode)
CLI->>FS: read script file
FS-->>CLI: JS source
CLI->>RDP: connect & getTarget()
CLI->>Firefox: eval(loaded JS)
Firefox-->>RDP: evaluation result
RDP-->>CLI: payload
CLI->>CLI: parse & format output
CLI-->>User: JSON result
sequenceDiagram
participant CLI as CLI (ff-rdp)
participant RDP as RDP Server
participant Watcher as Watcher Actor
participant Firefox as Firefox Tab
CLI->>RDP: connect & getWatcher()
RDP-->>CLI: watcher actor
CLI->>Watcher: watchResources("network-event")
CLI->>Firefox: reload()
loop poll until idle or timeout
Watcher-->>CLI: network-event batches or EOF
CLI->>CLI: accumulate requests_observed
CLI->>CLI: check idle_ms since last activity
end
CLI->>Watcher: unwatch_resources
CLI->>CLI: emit {reloaded, idle_at_ms, requests_observed}
CLI-->>User: JSON output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
kb/backlog/wait-console-includes.md (1)
25-25: Use the existing wait timeout flag name to avoid spec drift.Line 25 says
--timeout, but currentwaitimplementation uses--wait-timeout(crates/ff-rdp-cli/src/commands/wait.rs:11-55). This mismatch can cause implementation/docs inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kb/backlog/wait-console-includes.md` at line 25, The docs mention the flag name `--timeout` but the actual `wait` command uses `--wait-timeout`; update the markdown to reference the existing `--wait-timeout` flag (or alternatively update the `wait` command's argument parsing in wait.rs to accept `--timeout`) so docs and implementation match; look for the `wait` command definition and the `--wait-timeout` argument in wait.rs (and any associated variable like `wait_timeout`) and make the doc change to `--wait-timeout`.crates/ff-rdp-cli/tests/screenshot_e2e_test.rs (1)
631-662: Assert output artifact for--viewport-heightsuccess path.This test currently checks process success only. Add a file existence assertion (like the
--full-pagetest) so regressions in file writing are caught.Suggested test hardening
assert!( output.status.success(), "expected success, stderr: {}", String::from_utf8_lossy(&output.stderr) ); + assert!(out_path.exists(), "PNG file should have been written"); let _ = std::fs::remove_dir_all(&out_dir); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/tests/screenshot_e2e_test.rs` around lines 631 - 662, The test screenshot_viewport_height_flag_accepted currently only asserts the ff-rdp process exit status; after calling handle.join() and the success assertion, add an assertion that the expected output file (out_path) exists and is a regular file (e.g., check std::path::Path::exists()/is_file() on out_path) to mirror the --full-page test and catch regressions in file writing, then keep the existing cleanup (std::fs::remove_dir_all(&out_dir)).crates/ff-rdp-cli/tests/nav_action_e2e_test.rs (1)
74-81: Avoid relying onunknownMethodfor reload in success-path tests.Right now the wait-idle tests pass while reload returns an error from the mock. That can hide regressions in actual reload invocation. Prefer registering a normal
"reload"response in this server helper for the success scenarios.Also applies to: 83-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/tests/nav_action_e2e_test.rs` around lines 74 - 81, The mock server helper currently omits a successful "reload" handler and relies on the mock returning "unknownMethod" (which masks real regressions); fix by registering a proper reload response using the same pattern as the existing handler (e.g., add .on("reload", load_fixture("reload_response.json")) before .close_after_followups() in the helper where "unwatchResources" is registered), and apply the same change to the other similar server-helper blocks in this test file so all success-path tests have an explicit "reload" fixture response.crates/ff-rdp-cli/tests/computed_e2e_test.rs (1)
179-190: Please add one positive--allE2E case.Right now
--allis only covered as a conflict path. A happy-pathff-rdp computed ... --alltest would catch regressions in clap parsing, dispatch, and output shaping for the new flag.As per coding guidelines,
**/*.rs: Make Rust code unit testable and add tests when feasible; add e2e tests for all commands/subcommands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/tests/computed_e2e_test.rs` around lines 179 - 190, Add a new positive end-to-end test alongside computed_prop_and_all_conflict that invokes ff_rdp_bin() with args ["computed", "h1", "--all"], asserts the process exits successfully, and verifies the stdout contains the expected computed properties (at minimum check for a known field like "color" or another deterministic property from the fixture); name the test something like computed_all_happy_path or computed_prop_all_ok and reuse the same style of spawning and output inspection as the existing test to cover clap parsing, dispatch, and output shaping for the --all flag.crates/ff-rdp-cli/tests/eval_e2e_test.rs (1)
194-276: Assert the submitted JS, not just the JSON envelope.These two tests only prove that
eval --file/eval --stdinreach the handler and produce a normal response. They do not verify that the file/stdin contents are what gets sent toevaluateJSAsync, so a regression that drops or replaces the loaded source could still pass. IfMockRdpServercan inspect request params, please assert the evaluated script matches the temp-file/stdin payload.As per coding guidelines,
**/*.rs: Make Rust code unit testable and add tests when feasible; add e2e tests for all commands/subcommands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/tests/eval_e2e_test.rs` around lines 194 - 276, The tests eval_from_file and eval_from_stdin only check the JSON envelope; update them to also assert the actual JS sent to the RDP handler (evaluateJSAsync) matches the written file/stdin string: modify eval_server/MockRdpServer (or its returned server handle) to capture the incoming request params (the evaluated script), expose a method or field like last_evaluated_script or a get_last_request() on the server returned by eval_server, and after handle.join().unwrap() assert that last_evaluated_script == "getComputedStyle(document.body)?.display" (for the file test) and similarly for the stdin test (include the newline if written). Use the existing eval_server, MockRdpServer, and evaluateJSAsync identifiers to locate and hook into the request handling logic.crates/ff-rdp-cli/src/commands/computed.rs (1)
152-164: Avoid cloningentrieshere.
entriesis owned in this scope, so bothentries.clone()calls are avoidable. Computetotalfirst, then moveentriesinto the finalValue; that keeps this path aligned with the repo’s no-unnecessary-clone rule.As per coding guidelines,
**/*.rs: Noclone()unless the borrow checker demands it in Rust — try references first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/computed.rs` around lines 152 - 164, Compute total = entries.len() up front and then move entries instead of cloning: replace the entries.clone() usages and entries[0].clone() by using entries.into_iter().next().unwrap() for the single-item cases (so you can call .get("value").cloned().unwrap_or(Value::Null) on the moved first element when prop.is_some(), or return the moved element directly when prop.is_none()), and use Value::Array(entries) for the multi-item branches; this removes the unnecessary clones while preserving the same branching logic involving prop and results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/ff-rdp-cli/src/commands/eval.rs`:
- Around line 149-152: The test load_script_no_source_errors is currently
discarding the boolean result of matches!(err, AppError::User(_)); update the
assertion to actually assert the match (e.g., wrap matches! in assert! or use
assert!(matches!(...))) so that load_script(None, None, false).unwrap_err() is
verified to be an AppError::User; reference the test function
load_script_no_source_errors and the load_script call to locate the change.
---
Nitpick comments:
In `@crates/ff-rdp-cli/src/commands/computed.rs`:
- Around line 152-164: Compute total = entries.len() up front and then move
entries instead of cloning: replace the entries.clone() usages and
entries[0].clone() by using entries.into_iter().next().unwrap() for the
single-item cases (so you can call .get("value").cloned().unwrap_or(Value::Null)
on the moved first element when prop.is_some(), or return the moved element
directly when prop.is_none()), and use Value::Array(entries) for the multi-item
branches; this removes the unnecessary clones while preserving the same
branching logic involving prop and results.
In `@crates/ff-rdp-cli/tests/computed_e2e_test.rs`:
- Around line 179-190: Add a new positive end-to-end test alongside
computed_prop_and_all_conflict that invokes ff_rdp_bin() with args ["computed",
"h1", "--all"], asserts the process exits successfully, and verifies the stdout
contains the expected computed properties (at minimum check for a known field
like "color" or another deterministic property from the fixture); name the test
something like computed_all_happy_path or computed_prop_all_ok and reuse the
same style of spawning and output inspection as the existing test to cover clap
parsing, dispatch, and output shaping for the --all flag.
In `@crates/ff-rdp-cli/tests/eval_e2e_test.rs`:
- Around line 194-276: The tests eval_from_file and eval_from_stdin only check
the JSON envelope; update them to also assert the actual JS sent to the RDP
handler (evaluateJSAsync) matches the written file/stdin string: modify
eval_server/MockRdpServer (or its returned server handle) to capture the
incoming request params (the evaluated script), expose a method or field like
last_evaluated_script or a get_last_request() on the server returned by
eval_server, and after handle.join().unwrap() assert that last_evaluated_script
== "getComputedStyle(document.body)?.display" (for the file test) and similarly
for the stdin test (include the newline if written). Use the existing
eval_server, MockRdpServer, and evaluateJSAsync identifiers to locate and hook
into the request handling logic.
In `@crates/ff-rdp-cli/tests/nav_action_e2e_test.rs`:
- Around line 74-81: The mock server helper currently omits a successful
"reload" handler and relies on the mock returning "unknownMethod" (which masks
real regressions); fix by registering a proper reload response using the same
pattern as the existing handler (e.g., add .on("reload",
load_fixture("reload_response.json")) before .close_after_followups() in the
helper where "unwatchResources" is registered), and apply the same change to the
other similar server-helper blocks in this test file so all success-path tests
have an explicit "reload" fixture response.
In `@crates/ff-rdp-cli/tests/screenshot_e2e_test.rs`:
- Around line 631-662: The test screenshot_viewport_height_flag_accepted
currently only asserts the ff-rdp process exit status; after calling
handle.join() and the success assertion, add an assertion that the expected
output file (out_path) exists and is a regular file (e.g., check
std::path::Path::exists()/is_file() on out_path) to mirror the --full-page test
and catch regressions in file writing, then keep the existing cleanup
(std::fs::remove_dir_all(&out_dir)).
In `@kb/backlog/wait-console-includes.md`:
- Line 25: The docs mention the flag name `--timeout` but the actual `wait`
command uses `--wait-timeout`; update the markdown to reference the existing
`--wait-timeout` flag (or alternatively update the `wait` command's argument
parsing in wait.rs to accept `--timeout`) so docs and implementation match; look
for the `wait` command definition and the `--wait-timeout` argument in wait.rs
(and any associated variable like `wait_timeout`) and make the doc change to
`--wait-timeout`.
🪄 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
Run ID: 6025acf6-494b-452c-b02e-1c5621725891
📒 Files selected for processing (22)
README.mdcrates/ff-rdp-cli/src/cli/args.rscrates/ff-rdp-cli/src/commands/computed.rscrates/ff-rdp-cli/src/commands/console.rscrates/ff-rdp-cli/src/commands/eval.rscrates/ff-rdp-cli/src/commands/llm_help.rscrates/ff-rdp-cli/src/commands/mod.rscrates/ff-rdp-cli/src/commands/nav_action.rscrates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/dispatch.rscrates/ff-rdp-cli/tests/computed_e2e_test.rscrates/ff-rdp-cli/tests/console_e2e_test.rscrates/ff-rdp-cli/tests/eval_e2e_test.rscrates/ff-rdp-cli/tests/fixtures/eval_result_computed_empty.jsoncrates/ff-rdp-cli/tests/fixtures/eval_result_computed_multi.jsoncrates/ff-rdp-cli/tests/fixtures/eval_result_computed_prop.jsoncrates/ff-rdp-cli/tests/fixtures/eval_result_computed_single.jsoncrates/ff-rdp-cli/tests/fixtures/get_cached_messages_large_response.jsoncrates/ff-rdp-cli/tests/nav_action_e2e_test.rscrates/ff-rdp-cli/tests/screenshot_e2e_test.rskb/backlog/screenshot-annotate.mdkb/backlog/wait-console-includes.md
There was a problem hiding this comment.
Pull request overview
This PR focuses on pre-release developer-experience improvements to ff-rdp by adding safer input modes, new debugging commands, richer console output metadata, and more reliable navigation waits, with corresponding docs and e2e/fixture coverage.
Changes:
- Add new CLI capabilities:
eval --file/--stdin,screenshot --full-page/--viewport-height, newcomputedcommand, andreload --wait-idle. - Extend
consoleoutput with asummaryfield (total/matched/shown/by_level) and add fixtures/tests to validate behavior. - Update CLI help (
llm-help), README command list/examples, and add backlog notes for deferred features.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/backlog/wait-console-includes.md | Adds backlog note/spec for wait --console-includes. |
| kb/backlog/screenshot-annotate.md | Adds backlog note/spec for screenshot --annotate. |
| crates/ff-rdp-cli/tests/screenshot_e2e_test.rs | Adds e2e coverage for screenshot height modes and conflicts. |
| crates/ff-rdp-cli/tests/nav_action_e2e_test.rs | Adds e2e coverage for reload --wait-idle. |
| crates/ff-rdp-cli/tests/fixtures/get_cached_messages_large_response.json | Adds large console fixture used to validate limit/summary semantics. |
| crates/ff-rdp-cli/tests/fixtures/eval_result_computed_single.json | Adds computed-style eval fixture (single match). |
| crates/ff-rdp-cli/tests/fixtures/eval_result_computed_prop.json | Adds computed-style eval fixture (--prop). |
| crates/ff-rdp-cli/tests/fixtures/eval_result_computed_multi.json | Adds computed-style eval fixture (multi match). |
| crates/ff-rdp-cli/tests/fixtures/eval_result_computed_empty.json | Adds computed-style eval fixture (no matches). |
| crates/ff-rdp-cli/tests/eval_e2e_test.rs | Adds e2e coverage for eval --file/--stdin and argument validation. |
| crates/ff-rdp-cli/tests/console_e2e_test.rs | Adds e2e coverage for console.summary totals/limit/filter behavior. |
| crates/ff-rdp-cli/tests/computed_e2e_test.rs | Adds e2e coverage for new computed command. |
| crates/ff-rdp-cli/src/dispatch.rs | Wires new CLI flags/subcommands into dispatch. |
| crates/ff-rdp-cli/src/commands/screenshot.rs | Implements screenshot height overrides via generated JS + opts struct. |
| crates/ff-rdp-cli/src/commands/nav_action.rs | Implements run_reload_wait_idle network-idle drain loop. |
| crates/ff-rdp-cli/src/commands/mod.rs | Exposes new computed module. |
| crates/ff-rdp-cli/src/commands/llm_help.rs | Updates LLM-friendly help text for new flags/commands and console summary. |
| crates/ff-rdp-cli/src/commands/eval.rs | Adds load_script supporting positional/--file/--stdin. |
| crates/ff-rdp-cli/src/commands/console.rs | Adds summary field computation and insertion into output envelope. |
| crates/ff-rdp-cli/src/commands/computed.rs | Adds computed command implementation (JS builder + parsing + output shaping). |
| crates/ff-rdp-cli/src/cli/args.rs | Adds CLI surface for new flags/commands and updates long help strings. |
| README.md | Updates command list and usage examples for new features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+99
to
+112
| let mut requests_observed: u64 = 0; | ||
| let mut last_event_at = Instant::now(); | ||
|
|
||
| loop { | ||
| // Check total timeout first. | ||
| if start.elapsed() >= total_deadline { | ||
| break; | ||
| } | ||
|
|
||
| // Check idle threshold: if we've had at least one request and the last | ||
| // event was more than idle_ms ago, declare idle. | ||
| if requests_observed > 0 && last_event_at.elapsed() >= idle_threshold { | ||
| break; | ||
| } |
There was a problem hiding this comment.
--wait-idle should return once there has been no network activity for idle_ms, even if that means “no events ever arrived after reload”. The current requests_observed > 0 gate prevents the idle condition from ever triggering in the no-traffic case (it will wait until EOF/connection close or the total timeout). Consider initializing last_event_at = start and removing the requests_observed > 0 requirement so an empty-traffic reload can still complete after idle_ms.
Comment on lines
+120
to
+133
| if msg_type == "resources-available-array" || msg_type == "resources-updated-array" | ||
| { | ||
| // Count each individual network resource in the batch. | ||
| let count = msg | ||
| .get("array") | ||
| .and_then(serde_json::Value::as_array) | ||
| .map_or(1, |arr| { | ||
| arr.iter() | ||
| .filter_map(|pair| pair.as_array()) | ||
| .filter_map(|p| p.get(1)) | ||
| .filter_map(serde_json::Value::as_array) | ||
| .map(Vec::len) | ||
| .sum::<usize>() | ||
| }) as u64; |
There was a problem hiding this comment.
When counting network resources, map_or(1, ...) will add 1 request if a resources-*-array message is missing the array field (or has an unexpected shape). That inflates requests_observed and can make the idle logic behave incorrectly. Defaulting this to 0 (or ignoring the message when array is absent) is safer.
Comment on lines
+349
to
+370
| #[command(long_about = "Reload the page. | ||
|
|
||
| With --wait-idle, the command blocks after reload until network activity has been | ||
| idle for --idle-ms (default 500) or the --timeout expires (default 10000). | ||
|
|
||
| Examples: | ||
| ff-rdp reload | ||
| ff-rdp reload --wait-idle | ||
| ff-rdp reload --wait-idle --idle-ms 1000 --timeout 30000 | ||
|
|
||
| Output (plain): {\"results\": {\"action\": \"reload\"}, \"total\": 1, \"meta\": {...}} | ||
| Output (wait-idle): {\"results\": {\"reloaded\": true, \"idle_at_ms\": N, \"requests_observed\": M}, \"total\": 1, \"meta\": {...}}")] | ||
| Reload { | ||
| /// Block until network is idle after reload | ||
| #[arg(long)] | ||
| wait_idle: bool, | ||
| /// Milliseconds of network inactivity that counts as idle (--wait-idle only) | ||
| #[arg(long, default_value_t = 500)] | ||
| idle_ms: u64, | ||
| /// Maximum total milliseconds to wait for idle (--wait-idle only) | ||
| #[arg(long, default_value_t = 10000)] | ||
| reload_timeout: u64, |
There was a problem hiding this comment.
The reload long_help text and example reference a --timeout flag, but this subcommand defines --reload-timeout (and --timeout is a global option with different semantics). Update the help text/examples to use --reload-timeout to avoid misleading users.
Comment on lines
+33
to
+47
| fn build_js(selector: &str, prop: Option<&str>, include_all: bool) -> String { | ||
| let escaped_sel = escape_selector(selector); | ||
|
|
||
| if let Some(p) = prop { | ||
| // Escape the property name the same way as the selector — both end up | ||
| // inside single-quoted JS literals. | ||
| let escaped_prop = escape_selector(p); | ||
| return format!( | ||
| r"(function() {{ | ||
| var els = document.querySelectorAll('{escaped_sel}'); | ||
| var out = []; | ||
| for (var i = 0; i < els.length; i++) {{ | ||
| var cs = getComputedStyle(els[i]); | ||
| out.push({{selector: '{escaped_sel}', index: i, value: cs.getPropertyValue('{escaped_prop}') || cs['{escaped_prop}'] || ''}}); | ||
| }} |
There was a problem hiding this comment.
In --prop mode the output embeds selector: '{escaped_sel}', which means selectors containing ' will be returned with escape backslashes (e.g. div[data-x=\'y\']) rather than the exact selector the user passed. Since other commands (e.g. geometry/responsive) preserve the original selector string in output, consider keeping a separate “display selector” literal (e.g. JSON-encoded) for the selector field while still using the escaped value only for querySelectorAll.
Comment on lines
+67
to
+77
| document.body.appendChild(ref); | ||
| var rcs = getComputedStyle(ref); | ||
| var obj = {}; | ||
| for (var j = 0; j < cs.length; j++) { | ||
| var name = cs[j]; | ||
| var v = cs.getPropertyValue(name); | ||
| if (rcs.getPropertyValue(name) !== v) { | ||
| obj[name] = v; | ||
| } | ||
| } | ||
| ref.remove(); |
There was a problem hiding this comment.
The non-default filtering path assumes document.body exists (document.body.appendChild(ref)), which will throw on documents without a body (e.g. XML documents or early in navigation). Use a safer container (e.g. document.body || document.documentElement) or skip the default-diff optimization when there’s no body to attach to.
Suggested change
| document.body.appendChild(ref); | |
| var rcs = getComputedStyle(ref); | |
| var obj = {}; | |
| for (var j = 0; j < cs.length; j++) { | |
| var name = cs[j]; | |
| var v = cs.getPropertyValue(name); | |
| if (rcs.getPropertyValue(name) !== v) { | |
| obj[name] = v; | |
| } | |
| } | |
| ref.remove(); | |
| var container = document.body || document.documentElement; | |
| var rcs = null; | |
| if (container) { | |
| container.appendChild(ref); | |
| rcs = getComputedStyle(ref); | |
| } | |
| var obj = {}; | |
| for (var j = 0; j < cs.length; j++) { | |
| var name = cs[j]; | |
| var v = cs.getPropertyValue(name); | |
| if (!rcs || rcs.getPropertyValue(name) !== v) { | |
| obj[name] = v; | |
| } | |
| } | |
| if (container) { | |
| ref.remove(); | |
| } |
Comment on lines
+41
to
+43
| return std::fs::read_to_string(path) | ||
| .with_context(|| format!("eval: could not read script file '{path}'")) | ||
| .map_err(AppError::from); |
There was a problem hiding this comment.
load_script maps file read failures through anyhow into AppError::Internal, which makes a missing/unreadable --file path exit with code 2 and print internal error:. This is user input/path/permission failure and should be surfaced as AppError::User (exit 1) with the same helpful message.
Suggested change
| return std::fs::read_to_string(path) | |
| .with_context(|| format!("eval: could not read script file '{path}'")) | |
| .map_err(AppError::from); | |
| return std::fs::read_to_string(path).map_err(|e| { | |
| AppError::User(format!("eval: could not read script file '{path}': {e}")) | |
| }); |
| #[test] | ||
| fn load_script_no_source_errors() { | ||
| let err = load_script(None, None, false).unwrap_err(); | ||
| matches!(err, AppError::User(_)); |
There was a problem hiding this comment.
This test currently computes matches!(err, AppError::User(_)) but doesn’t assert the result, so it will pass even if load_script returns the wrong error variant. Wrap this in assert!(matches!(...)) (or equivalent) so the invariant is actually tested.
Suggested change
| matches!(err, AppError::User(_)); | |
| assert!(matches!(err, AppError::User(_))); |
- Fix discarded matches!() in load_script test (now assert!) - Map file-read errors to AppError::User instead of Internal - Reduce ScreenshotOpts visibility to pub(crate) - Eliminate unnecessary clones in computed.rs by moving total above match - Rename JS var ref to refEl (reserved word) and guard document.body - Remove requests_observed>0 gate so zero-traffic pages exit after idle_ms - Use map_or(0,...) for missing array field and guard last_event_at update - Fix help text: --timeout → --reload-timeout in reload examples - Add requires="wait_idle" constraint on --idle-ms and --reload-timeout - Assert PNG file exists in screenshot_viewport_height test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Last pre-release iteration. Addresses the concrete friction surfaced in the nova-template dogfooding session — each fix is scoped, cheap, and removes an "I kept reinventing this" reflex a first-time user would hit in minutes.
eval --file/--stdin— sidesteps shell-level quoting gotchas (optional chaining, template literals, multi-statement JS) that made users rewrite perfectly valid JS.screenshot --full-page/--viewport-height— captures the whole scrollable document instead of just the first fold.computedcommand — first-class wrapper aroundgetComputedStyle(), the most-reused eval snippet in CSS debugging. Mirrorsdom's multi-match shape ({selector, index, computed: {...}}).consolesummary field — output now includes{total, matched, shown, by_level}so callers can tell at a glance whether the filter caught what they expected.--limitaudited and confirmed working.reload --wait-idle— blocks until no network activity for--idle-ms(default 500) or--timeoutexpires, replacing flakysleep 5hacks.navigatelong_about — clarifies that URL is positional; no--urlflag exists.wait --console-includesandscreenshot --annotatedeferred with kb/backlog notes.Docs: README.md command table +
llm-helpreference updated.Test plan
cargo fmt --checkcleancargo clippy --workspace --all-targets -- -D warningscleancargo test --workspaceall green (includes new e2e tests for each command)evalnew tests: from-file, from-stdin, missing-source error, conflicting-sources errorscreenshotnew tests:--full-pageJS wiring,--viewport-heightJS wiring, conflict errorcomputednew tests: single-match object, multi-match array,--propscalar, no-match error,--jqfilter,--prop + --allconflictconsolenew tests: summary present,--limit 10on 110-message fixture yieldsshown=10withtotalreflecting full count,matchedreflects filterreload --wait-idletests: observes network events; returns quickly when idle🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
computedcommand (--prop/--all), richerevalinput modes (--file,--stdin), screenshot--full-page/--viewport-height, andreload --wait-idlewith idle/timeoutssummary(total, matched, shown, per-level)Tests
computed,evalmodes,consolesummary,screenshotflags, andreload --wait-idleDocumentation