Fix dogfooding issues: full-page screenshots, text formatters, scroll UX (iter-45)#47
Merged
Fix dogfooding issues: full-page screenshots, text formatters, scroll UX (iter-45)#47
Conversation
… UX (iter-45) Fixes found in dogfooding session 37 on sbb.ch: - Pass fullPage param through to RDP screenshot actor (was silently ignored) - Add --format text renderers for perf summary, a11y tree, and responsive - Wire --limit through a11y and sources commands - Fix scroll by --dy negative number parsing (allow_negative_numbers) - Add scroll top / scroll bottom subcommands - Fix reload --wait-idle reporting 0 requests (defer idle check until first event) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds scroll top/bottom subcommands and negative-scroll parsing; implements human-readable Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Daemon as "RDP Daemon / Transport"
participant Content as "ScreenshotContentActor"
participant Screenshot as "ScreenshotActor"
participant Output as "OutputPipeline / stdout"
CLI->>Daemon: connect & request screenshot (full_page flag)
Daemon->>Content: forward capture request (fullPage: true|false)
Content->>Screenshot: prepare_capture(fullPage)
Screenshot-->>Content: capture result (image / metadata)
Content-->>Daemon: return capture payload
Daemon-->>CLI: deliver JSON result
CLI->>Output: envelope / render and write to stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses several CLI UX and correctness issues found during dogfooding (session 37), improving screenshot behavior, output formatting consistency, scroll ergonomics, limit handling, and reload idle detection.
Changes:
- Forward
--full-pagethrough to the underlying RDP screenshot actors (legacy + two-step) and add a core-level request assertion test. - Add human-readable
--format textrenderers forperf summary,a11y, andresponsive. - Improve scroll UX (negative
--dx/--dyparsing; addscroll top/scroll bottom) and adjustreload --wait-idleidle timing.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-45-dogfood-fixes.md | Adds Iteration 45 planning doc capturing dogfooding issues and acceptance criteria. |
| kb/iterations/iteration-44-public-release.md | Marks iteration 44 as completed. |
| kb/dogfooding/dogfooding-session-37.md | Adds the dogfooding session 37 report that motivates this work. |
| crates/ff-rdp-core/src/actors/screenshot_content.rs | Adds full_page param plumbing to legacy capture + tests asserting request payload. |
| crates/ff-rdp-cli/tests/nav_action_e2e_test.rs | Updates test commentary to reflect revised idle-loop semantics. |
| crates/ff-rdp-cli/src/dispatch.rs | Wires new scroll top / scroll bottom subcommands into dispatch. |
| crates/ff-rdp-cli/src/commands/sources.rs | Applies --limit/--all to sources results via OutputControls and truncation envelope. |
| crates/ff-rdp-cli/src/commands/scroll.rs | Implements scroll top/bottom execution + adds tests around new scroll functionality. |
| crates/ff-rdp-cli/src/commands/screenshot.rs | Forwards full-page intent into both screenshot protocols. |
| crates/ff-rdp-cli/src/commands/responsive.rs | Adds a text-mode renderer for responsive breakpoint results. |
| crates/ff-rdp-cli/src/commands/perf.rs | Adds a text-mode renderer for perf summary. |
| crates/ff-rdp-cli/src/commands/nav_action.rs | Adjusts reload --wait-idle idle timing to avoid premature idle detection. |
| crates/ff-rdp-cli/src/commands/llm_help.rs | Updates LLM help to document negative scroll values and new scroll subcommands. |
| crates/ff-rdp-cli/src/commands/a11y.rs | Adds text tree rendering and applies limiting by flattening the tree when requested. |
| crates/ff-rdp-cli/src/cli/args.rs | Enables negative number parsing for scroll by and adds new scroll subcommands to clap model. |
| .claude/skills/dogfood/SKILL.md | Adds a dogfooding “skill” doc for running structured CLI dogfood sessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+543
to
+561
| fn run_by_negative_dy_produces_negative_js_expr() { | ||
| // Negative dy values must produce a negative literal in the JS expression | ||
| // (i.e. "scroll by --dy -500" should scroll up, not fail parsing). | ||
| let dy: i64 = -500; | ||
| let dy_expr = dy.to_string(); | ||
| assert_eq!(dy_expr, "-500"); | ||
| assert!(dy_expr.starts_with('-')); | ||
| } | ||
|
|
||
| #[test] | ||
| fn run_by_negative_dx_produces_negative_js_expr() { | ||
| let dx: i64 = -200; | ||
| let js = format!( | ||
| r"(function() {{ | ||
| window.scrollBy({{left: {dx}, top: 0, behavior: 'auto'}}); | ||
| return true; | ||
| }})()" | ||
| ); | ||
| assert!(js.contains("left: -200")); |
Comment on lines
+100
to
+127
| // `last_event_at` is None until the first network event arrives. | ||
| // | ||
| // # Timing fix | ||
| // | ||
| // Previously this was initialised to `Instant::now()` (before the reload | ||
| // fires), which caused the idle check to trigger immediately if no events | ||
| // arrived within `idle_ms` of the *watch subscription* — well before the | ||
| // reloaded page had time to generate any requests. Pages that take longer | ||
| // than `idle_ms` to produce their first network event would therefore always | ||
| // report 0 requests. | ||
| // | ||
| // The idle threshold should only apply *after the first event is seen*: | ||
| // - Before any event: only the total timeout governs termination. | ||
| // - After the first event: terminate when no new event arrives for idle_ms. | ||
| let mut last_event_at: Option<Instant> = None; | ||
|
|
||
| loop { | ||
| // Check total timeout first. | ||
| if start.elapsed() >= total_deadline { | ||
| break; | ||
| } | ||
|
|
||
| // Check idle threshold: if the last event (or start) was more than | ||
| // idle_ms ago, declare idle. This handles both the common case (real | ||
| // requests observed) and zero-traffic pages (fully cached). | ||
| if last_event_at.elapsed() >= idle_threshold { | ||
| // Check idle threshold only after at least one network event has been | ||
| // received. Before that, the reload may still be in-flight (DNS, | ||
| // TCP handshake, page teardown) and we must not declare idle too early. | ||
| if let Some(t) = last_event_at | ||
| && t.elapsed() >= idle_threshold | ||
| { |
Comment on lines
+114
to
+118
| // Apply --limit / --all output controls (no default limit for sources). | ||
| let controls = OutputControls::from_cli(cli, SortDir::Asc); | ||
| let (limited, total, truncated) = controls.apply_limit(results, None); | ||
| let shown = limited.len(); | ||
| let result_json = json!(limited); |
Comment on lines
+81
to
+83
| let (limited, total, truncated) = controls.apply_limit(flat, None); | ||
| let shown = limited.len(); | ||
| output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ff-rdp-cli/src/commands/nav_action.rs (1)
122-154:⚠️ Potential issue | 🟠 MajorZero-traffic reloads now wait until
--reload-timeout.Because
last_event_atis only armed by a non-empty"network-event"batch, a page that genuinely reloads without network traffic never becomes “idle” here. On a live RDP connection that meansreload --wait-idleblocks until EOF or the total timeout, even if the reload finished long before that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/nav_action.rs` around lines 122 - 154, The loop only arms last_event_at when a resources batch contains >0 items, so zero-traffic reloads never mark "network event seen" and thus never become idle; update the handling inside the match for msg_type == "resources-available-array" || "resources-updated-array" to set last_event_at = Some(Instant::now()) whenever such a batch message is received (regardless of the computed count), and keep the existing requests_observed += count logic unchanged so count still reflects actual resources.
🧹 Nitpick comments (2)
crates/ff-rdp-cli/src/commands/sources.rs (1)
114-119: Apply full output controls for consistency (--sort,--fields)Nice fix for
--limit/--all. To align with other list commands and global control semantics, apply sort + fields in the same block.♻️ Suggested refactor
- let (limited, total, truncated) = controls.apply_limit(results, None); + let mut results = results; + controls.apply_sort(&mut results); + let (limited, total, truncated) = controls.apply_limit(results, None); let shown = limited.len(); - let result_json = json!(limited); + let limited = controls.apply_fields(limited); + let result_json = json!(limited);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/sources.rs` around lines 114 - 119, The current block only applies controls.apply_limit but skips sort/fields; update the sequence so OutputControls::from_cli(cli, SortDir::Asc) is used to first apply sorting and field selection to results (call the controls' apply_sort and apply_fields methods or the combined apply_sort_and_fields helper if available) before calling controls.apply_limit; then compute shown/result_json/meta from the final limited/filtered list (keep using controls.apply_limit, json!(limited), and meta as before).crates/ff-rdp-cli/src/commands/scroll.rs (1)
158-166: Use the page’s scrolling root forscroll bottom.
document.documentElement.scrollHeightis brittle on pages wherebody/scrollingElementowns scrolling. This can stop short on quirks-mode or body-scrolling documents.Suggested hardening
- window.scrollTo(0, document.documentElement.scrollHeight); + var root = document.scrollingElement || document.documentElement || document.body; + window.scrollTo(0, root ? root.scrollHeight : 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/scroll.rs` around lines 158 - 166, The injected JS currently assumes document.documentElement is the scrolling root (the js string built in scroll.rs uses document.documentElement.scrollHeight and window.scrollY); change it to detect and use the page's scrolling root: create a scrollingRoot variable like document.scrollingElement || document.documentElement || document.body, call scrollingRoot.scrollTo(0, scrollingRoot.scrollHeight) (or fall back to window.scrollTo), and replace document.documentElement.scrollHeight, window.scrollY, and window.scrollX with scrollingRoot.scrollHeight, scrollingRoot.scrollTop, and scrollingRoot.scrollLeft respectively (also use scrollingRoot.scrollTop + window.innerHeight to compute atEnd) so the scroll-bottom logic works for body- or scrollingElement-owned scrolls.
🤖 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/cli/args.rs`:
- Around line 611-620: Add end-to-end tests for the new Top and Bottom
subcommands by following the pattern used in scroll_e2e_test.rs: create two
tests (e.g., scroll_top_e2e_test and scroll_bottom_e2e_test) that parse the CLI
into the Scroll enum variants (Top and Bottom), dispatch them through the same
command execution path, and assert the output envelope JSON contains the
expected fields (results.scrolled, results.viewport, results.scrollHeight,
results.atEnd, total, meta). Locate the existing scroll_e2e_test.rs to mirror
setup, invocation helpers, and JSON assertion helpers so the new tests validate
parse, dispatch, and output exactly like the other scroll subcommand tests.
In `@crates/ff-rdp-cli/src/commands/a11y.rs`:
- Around line 74-94: Tests are missing for the new a11y output modes: add
integration/CLI tests that exercise the command-level behavior when cli.format
== "text" and when cli.limit / cli.all are used so we assert the rendered
indented tree and the truncated flat-list envelope respectively. Write tests
that invoke the CLI/subcommand (the code paths that call
render_a11y_text(&tree_value, 0) and the branch that builds
OutputControls::from_cli(...) and calls apply_limit(...) and
output::envelope_with_truncation(...)), run the binary or call the top-level
command runner, and assert the stdout contains expected tree lines for the text
mode and contains the limited items plus truncation/total metadata for the
limit/all mode; add corresponding e2e/integration tests alongside other command
tests covering the affected command paths (also cover the other changes around
the nearby block lines referenced).
In `@crates/ff-rdp-cli/src/commands/perf.rs`:
- Around line 657-661: The current early return in perf.rs (checking cli.format
== "text" && cli.jq.is_none() and calling render_summary_text(&results)) breaks
the JSON-only output contract; change the flow so the summary is produced as
data and serialized into the standard JSON envelope instead of writing raw text
directly. Concretely, refactor render_summary_text (or add a new helper like
render_summary_string) to return the summary as a String (or structured value)
rather than printing, then replace the short-circuit branch to build the usual
JSON output object (including a "summary" field or similar) and emit it with
serde_json::to_writer()/to_string(), ensuring the pipeline remains JSON-only and
still supports cli.jq handling for the "perf summary" path.
In `@crates/ff-rdp-cli/src/commands/screenshot.rs`:
- Around line 111-124: Add an end-to-end test that runs the CLI "screenshot
--full-page" (the screenshot subcommand implemented in screenshot.rs) exercising
both protocol paths (the path that calls ScreenshotContentActor::capture and the
fallback try_two_step_screenshot) by driving a test page taller than the
viewport and asserting the produced image's pixel height is greater than the
viewport height; ensure the test uses the same HeightOverride::FullPage flag
plumbing, covers the legacy and two-step code paths (simulate or force both
where possible), and places the test alongside other e2e command tests so it
exercises the full command-level behavior rather than only lower-level request
shaping.
In `@crates/ff-rdp-cli/src/commands/scroll.rs`:
- Around line 542-588: Current tests only assert JS string generation and miss
CLI parsing/dispatch regressions; add parser/unit tests that exercise the clap
parsing and end-to-end tests that invoke the command dispatch. Specifically, add
a unit test that uses clap::Command (or the crate's argument-parsing entry, e.g.
try_get_matches_from) to parse ["scroll","by","--dy","-500"] and assert the
parsed value for "dy" is -500 (augment or replace
run_by_negative_dy_produces_negative_js_expr), and add e2e tests (using
assert_cmd or the integration test harness to run the built binary) that run
"scroll top" and "scroll bottom" and assert the process output/behavior contains
the JSON_SENTINEL or the expected scroll JS (augment
run_top_js_scrolls_to_origin and run_bottom_js_scrolls_to_scroll_height) so
parsing, dispatch, and runtime wiring are validated end-to-end.
In `@kb/dogfooding/dogfooding-session-37.md`:
- Around line 103-106: Update the summary bullet block so its numeric roll-up
matches the detailed findings in the report body: recount the total commands
tested and the exact counts of clean passes, high-severity bugs (e.g.,
`screenshot --full-page`), and minor issues referenced elsewhere (including the
"Cookies regression fixed"/session 32 TypeError and the new features notes for
`scroll`, `eval --file/--stdin`, `computed`), then replace the three summary
lines so the totals align with the enumerated items and adjust the "Key
takeaway" sentence to reflect the corrected counts.
---
Outside diff comments:
In `@crates/ff-rdp-cli/src/commands/nav_action.rs`:
- Around line 122-154: The loop only arms last_event_at when a resources batch
contains >0 items, so zero-traffic reloads never mark "network event seen" and
thus never become idle; update the handling inside the match for msg_type ==
"resources-available-array" || "resources-updated-array" to set last_event_at =
Some(Instant::now()) whenever such a batch message is received (regardless of
the computed count), and keep the existing requests_observed += count logic
unchanged so count still reflects actual resources.
---
Nitpick comments:
In `@crates/ff-rdp-cli/src/commands/scroll.rs`:
- Around line 158-166: The injected JS currently assumes
document.documentElement is the scrolling root (the js string built in scroll.rs
uses document.documentElement.scrollHeight and window.scrollY); change it to
detect and use the page's scrolling root: create a scrollingRoot variable like
document.scrollingElement || document.documentElement || document.body, call
scrollingRoot.scrollTo(0, scrollingRoot.scrollHeight) (or fall back to
window.scrollTo), and replace document.documentElement.scrollHeight,
window.scrollY, and window.scrollX with scrollingRoot.scrollHeight,
scrollingRoot.scrollTop, and scrollingRoot.scrollLeft respectively (also use
scrollingRoot.scrollTop + window.innerHeight to compute atEnd) so the
scroll-bottom logic works for body- or scrollingElement-owned scrolls.
In `@crates/ff-rdp-cli/src/commands/sources.rs`:
- Around line 114-119: The current block only applies controls.apply_limit but
skips sort/fields; update the sequence so OutputControls::from_cli(cli,
SortDir::Asc) is used to first apply sorting and field selection to results
(call the controls' apply_sort and apply_fields methods or the combined
apply_sort_and_fields helper if available) before calling controls.apply_limit;
then compute shown/result_json/meta from the final limited/filtered list (keep
using controls.apply_limit, json!(limited), and meta as before).
🪄 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: 44a18c20-ac4f-44cb-9759-c2b42bdafae8
📒 Files selected for processing (16)
.claude/skills/dogfood/SKILL.mdcrates/ff-rdp-cli/src/cli/args.rscrates/ff-rdp-cli/src/commands/a11y.rscrates/ff-rdp-cli/src/commands/llm_help.rscrates/ff-rdp-cli/src/commands/nav_action.rscrates/ff-rdp-cli/src/commands/perf.rscrates/ff-rdp-cli/src/commands/responsive.rscrates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/commands/scroll.rscrates/ff-rdp-cli/src/commands/sources.rscrates/ff-rdp-cli/src/dispatch.rscrates/ff-rdp-cli/tests/nav_action_e2e_test.rscrates/ff-rdp-core/src/actors/screenshot_content.rskb/dogfooding/dogfooding-session-37.mdkb/iterations/iteration-44-public-release.mdkb/iterations/iteration-45-dogfood-fixes.md
Comment on lines
+74
to
+94
| // When --limit / --all is set, flatten the tree into a list of nodes and | ||
| // apply the limit. Without a limit flag the output remains a single tree | ||
| // object (the historical default behaviour). | ||
| let controls = OutputControls::from_cli(cli, SortDir::Asc); | ||
| let envelope = if cli.limit.is_some() || cli.all { | ||
| let mut flat = Vec::new(); | ||
| flatten_tree(&tree_value, &mut flat); | ||
| let (limited, total, truncated) = controls.apply_limit(flat, None); | ||
| let shown = limited.len(); | ||
| output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta) | ||
| } else { | ||
| output::envelope(&tree_value, 1, &meta) | ||
| }; | ||
|
|
||
| // Text short-circuit: render an indented accessibility tree instead of JSON. | ||
| // When --limit / --all is active we fall through to the pipeline which will | ||
| // render the flat list as a table via the generic text renderer. | ||
| if cli.format == "text" && cli.jq.is_none() && cli.limit.is_none() && !cli.all { | ||
| render_a11y_text(&tree_value, 0); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The new a11y output modes need command-level coverage.
--format text and --limit/--all are now user-visible behavior changes, but the added tests only exercise helper internals and “does not panic” cases. Please add CLI/e2e assertions for the rendered text tree and the truncated flat-list output.
As per coding guidelines "Make Rust code unit testable and add tests when feasible; add e2e tests for all commands/subcommands".
Also applies to: 510-604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ff-rdp-cli/src/commands/a11y.rs` around lines 74 - 94, Tests are
missing for the new a11y output modes: add integration/CLI tests that exercise
the command-level behavior when cli.format == "text" and when cli.limit /
cli.all are used so we assert the rendered indented tree and the truncated
flat-list envelope respectively. Write tests that invoke the CLI/subcommand (the
code paths that call render_a11y_text(&tree_value, 0) and the branch that builds
OutputControls::from_cli(...) and calls apply_limit(...) and
output::envelope_with_truncation(...)), run the binary or call the top-level
command runner, and assert the stdout contains expected tree lines for the text
mode and contains the limited items plus truncation/total metadata for the
limit/all mode; add corresponding e2e/integration tests alongside other command
tests covering the affected command paths (also cover the other changes around
the nearby block lines referenced).
Comment on lines
+657
to
+661
| // Text short-circuit: render a human-readable summary table instead of JSON. | ||
| if cli.format == "text" && cli.jq.is_none() { | ||
| render_summary_text(&results); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
perf summary text short-circuit violates the JSON-only output contract.
This branch bypasses the JSON envelope/pipeline and emits raw text directly, which diverges from the CLI output contract required for Rust commands.
As per coding guidelines "Output JSON-only with --jq filter support in Rust CLI applications".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ff-rdp-cli/src/commands/perf.rs` around lines 657 - 661, The current
early return in perf.rs (checking cli.format == "text" && cli.jq.is_none() and
calling render_summary_text(&results)) breaks the JSON-only output contract;
change the flow so the summary is produced as data and serialized into the
standard JSON envelope instead of writing raw text directly. Concretely,
refactor render_summary_text (or add a new helper like render_summary_string) to
return the summary as a String (or structured value) rather than printing, then
replace the short-circuit branch to build the usual JSON output object
(including a "summary" field or similar) and emit it with
serde_json::to_writer()/to_string(), ensuring the pipeline remains JSON-only and
still supports cli.jq handling for the "perf summary" path.
Comment on lines
+111
to
+124
| match ScreenshotContentActor::capture( | ||
| ctx.transport_mut(), | ||
| actor.as_ref(), | ||
| height_override.is_some_and(|h| matches!(h, HeightOverride::FullPage)), | ||
| ) { | ||
| Ok(capture) => capture.data, | ||
| Err(legacy_err) if legacy_err.is_unrecognized_packet_type() => { | ||
| // Legacy method not available — try the Firefox 149+ two-step protocol. | ||
| try_two_step_screenshot(&mut ctx, &actor, browsing_ctx_id)? | ||
| try_two_step_screenshot( | ||
| &mut ctx, | ||
| &actor, | ||
| browsing_ctx_id, | ||
| height_override.is_some_and(|h| matches!(h, HeightOverride::FullPage)), | ||
| )? |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Please add an end-to-end test for screenshot --full-page.
This fixes the flag plumbing in both screenshot protocol paths, but the regression was command-level and the current coverage only proves lower-level request shaping. An e2e test that asserts the output image is taller than the viewport would protect the real behavior that broke.
As per coding guidelines "Make Rust code unit testable and add tests when feasible; add e2e tests for all commands/subcommands".
Also applies to: 203-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ff-rdp-cli/src/commands/screenshot.rs` around lines 111 - 124, Add an
end-to-end test that runs the CLI "screenshot --full-page" (the screenshot
subcommand implemented in screenshot.rs) exercising both protocol paths (the
path that calls ScreenshotContentActor::capture and the fallback
try_two_step_screenshot) by driving a test page taller than the viewport and
asserting the produced image's pixel height is greater than the viewport height;
ensure the test uses the same HeightOverride::FullPage flag plumbing, covers the
legacy and two-step code paths (simulate or force both where possible), and
places the test alongside other e2e command tests so it exercises the full
command-level behavior rather than only lower-level request shaping.
…ization Address Copilot + CodeRabbit + local review findings: - Add clap parse tests for scroll --dy -500, scroll top/bottom, screenshot --full-page, a11y --limit - Add e2e tests for scroll top and scroll bottom subcommands - Wire sort/fields through OutputControls in a11y and sources (were silently ignored) - Add early-exit to flatten_tree when --limit is set (avoids cloning entire tree) - Extract run_scroll_absolute helper to deduplicate run_top/run_bottom - Fix dogfooding session 37 summary counts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/a11y.rs`:
- Around line 78-90: When flatten_tree is allowed to stop early (early_stop set
from cli.limit/controls.all) the code misreports total/truncated because
apply_limit only sees the truncated flat.len(); update this block to detect
early exit (e.g., have flatten_tree return a flag or a count hint) and pass the
real-total-or-unknown info into output::envelope_with_truncation instead of
using the second value from controls.apply_limit as the true total;
alternatively perform a quick counting pass before the limited flatten or change
OutputControls::apply_limit to accept an early_exit boolean so
envelope_with_truncation and the total/truncated values are computed correctly
for flatten_tree/controls.apply_limit/apply_fields.
🪄 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: 2b4ed9bc-003f-43dd-8f32-414843d7cfa5
📒 Files selected for processing (6)
crates/ff-rdp-cli/src/commands/a11y.rscrates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/commands/scroll.rscrates/ff-rdp-cli/src/commands/sources.rscrates/ff-rdp-cli/tests/scroll_e2e_test.rskb/dogfooding/dogfooding-session-37.md
✅ Files skipped from review due to trivial changes (1)
- kb/dogfooding/dogfooding-session-37.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/ff-rdp-cli/src/commands/sources.rs
- crates/ff-rdp-cli/src/commands/scroll.rs
- crates/ff-rdp-cli/src/commands/screenshot.rs
Comment on lines
+78
to
+90
| let envelope = if cli.limit.is_some() || cli.all { | ||
| // Pass the limit so flatten_tree can stop early instead of cloning all nodes. | ||
| let early_stop = if controls.all { None } else { controls.limit }; | ||
| let mut flat = Vec::new(); | ||
| flatten_tree(&tree_value, &mut flat, early_stop); | ||
| controls.apply_sort(&mut flat); | ||
| let (limited, total, truncated) = controls.apply_limit(flat, None); | ||
| let limited = controls.apply_fields(limited); | ||
| let shown = limited.len(); | ||
| output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta) | ||
| } else { | ||
| output::envelope(&tree_value, 1, &meta) | ||
| }; |
There was a problem hiding this comment.
Truncation metadata is incorrect when --limit triggers early exit.
The early-exit optimization at line 80 conflicts with accurate truncation reporting. When flatten_tree stops early at limit nodes, apply_limit sees flat.len() == limit, computes limit >= total, and returns truncated = false.
Example: tree has 100 nodes, --limit 10:
flatten_treecollects 10 nodes and stopsapply_limitsees 10 items, limit=10, returns(items, 10, false)- Envelope reports
"total": 10, "truncated": false— but the tree actually has 100 nodes
Additionally, line 84 names the second return value total, but per OutputControls::apply_limit it's the limit used, not the original count.
Consider tracking whether early exit occurred and passing the true total (or a flag indicating "unknown total due to early exit") to envelope_with_truncation.
🛠️ Suggested approach
- let early_stop = if controls.all { None } else { controls.limit };
- let mut flat = Vec::new();
- flatten_tree(&tree_value, &mut flat, early_stop);
- controls.apply_sort(&mut flat);
- let (limited, total, truncated) = controls.apply_limit(flat, None);
- let limited = controls.apply_fields(limited);
- let shown = limited.len();
- output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta)
+ // Flatten without early exit to get accurate total count.
+ let mut flat = Vec::new();
+ flatten_tree(&tree_value, &mut flat, None);
+ let total_count = flat.len();
+ controls.apply_sort(&mut flat);
+ let (limited, _limit, truncated) = controls.apply_limit(flat, None);
+ let limited = controls.apply_fields(limited);
+ let shown = limited.len();
+ output::envelope_with_truncation(&json!(limited), shown, total_count, truncated, &meta)If the early-exit optimization is important for very large trees, consider a two-pass approach: first count nodes, then flatten with limit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let envelope = if cli.limit.is_some() || cli.all { | |
| // Pass the limit so flatten_tree can stop early instead of cloning all nodes. | |
| let early_stop = if controls.all { None } else { controls.limit }; | |
| let mut flat = Vec::new(); | |
| flatten_tree(&tree_value, &mut flat, early_stop); | |
| controls.apply_sort(&mut flat); | |
| let (limited, total, truncated) = controls.apply_limit(flat, None); | |
| let limited = controls.apply_fields(limited); | |
| let shown = limited.len(); | |
| output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta) | |
| } else { | |
| output::envelope(&tree_value, 1, &meta) | |
| }; | |
| let envelope = if cli.limit.is_some() || cli.all { | |
| // Flatten without early exit to get accurate total count. | |
| let mut flat = Vec::new(); | |
| flatten_tree(&tree_value, &mut flat, None); | |
| let total_count = flat.len(); | |
| controls.apply_sort(&mut flat); | |
| let (limited, _limit, truncated) = controls.apply_limit(flat, None); | |
| let limited = controls.apply_fields(limited); | |
| let shown = limited.len(); | |
| output::envelope_with_truncation(&json!(limited), shown, total_count, truncated, &meta) | |
| } else { | |
| output::envelope(&tree_value, 1, &meta) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ff-rdp-cli/src/commands/a11y.rs` around lines 78 - 90, When
flatten_tree is allowed to stop early (early_stop set from
cli.limit/controls.all) the code misreports total/truncated because apply_limit
only sees the truncated flat.len(); update this block to detect early exit
(e.g., have flatten_tree return a flag or a count hint) and pass the
real-total-or-unknown info into output::envelope_with_truncation instead of
using the second value from controls.apply_limit as the true total;
alternatively perform a quick counting pass before the limited flatten or change
OutputControls::apply_limit to accept an early_exit boolean so
envelope_with_truncation and the total/truncated values are computed correctly
for flatten_tree/controls.apply_limit/apply_fields.
- nav_action: arm idle timer on every watcher batch, not just non-empty ones — fixes zero-traffic reloads blocking until total timeout - scroll: use document.scrollingElement fallback for scroll bottom, supporting body-scrolling and quirks-mode pages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reduces verbose test output that wastes tokens and context window. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Iteration 46: consolidate 29 e2e test binaries into single binary - Add recorded scroll top/bottom test fixtures Co-Authored-By: Claude Opus 4.6 (1M context) <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
fullPageparam through to RDP actor (was silently ignored)perf summary,a11y, andresponsivea11yandsourcescommands viaOutputControlsallow_negative_numbers = trueso--dy -500worksAll issues found in dogfooding session 37 on sbb.ch.
Test plan
cargo fmt && cargo clippy --workspace --all-targets -- -D warnings && cargo test --workspace— all passff-rdp screenshot --full-pageon a scrollable page produces taller-than-viewport imageff-rdp perf summary --format text/a11y --format text/responsive --format textproduce tablesff-rdp scroll by --dy -500works without equals syntaxff-rdp scroll topandscroll bottomworkff-rdp reload --wait-idlereports >0 requests on a real page🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--format textoutput for accessibility, performance summary, and responsive reportsscroll byaccepts negative values for upward/backward scrolling--limithonored in accessibility and sources outputsBug Fixes
Documentation