Merged
Conversation
1. launch: auto-create temp profile with devtools prefs when no --profile/--temp-profile given, so debugger server always starts 2. scroll: use requestAnimationFrame to read post-scroll position, fixing stale viewport.y reporting in top/bottom/by/to/container 3. text formatters: add --format text for geometry (tabular), network summary (sections), and dom tree (indented tree) 4. reload --wait-idle: already fixed (prior iter); task verified 5. responsive: replace fixed 50ms sleep with JS layout stabilization (rAF + setTimeout) to prevent negative rect.y on complex pages 6. recipes/llm-help: hide irrelevant global flags via help_template Co-Authored-By: Claude Opus 4.6 (1M context) <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 (4)
📝 WalkthroughWalkthroughAdds text-format short-circuits for geometry/network/dom-tree, auto-creates temporary Firefox profiles with required user.js prefs in launch, replaces fixed sleep with requestAnimationFrame-based layout wait in responsive, defers scroll measurements to rAF Promises, hides global flags in static help, and adds related e2e tests and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (1)
crates/ff-rdp-cli/src/commands/launch.rs (1)
246-271: Consider extracting shared temp-profile creation logic.The code in this
elsebranch (lines 247-270) is nearly identical to theelse if temp_profilebranch (lines 225-245). Both create a temp directory, writeUSER_JS, and add--profileto the command.This duplication is minor but could be consolidated into a helper function.
♻️ Suggested refactor to reduce duplication
fn create_temp_profile_dir() -> Result<PathBuf, AppError> { let nonce = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_micros()) .unwrap_or(0); let tmp = std::env::temp_dir().join(format!("ff-rdp-profile-{}-{nonce}", std::process::id())); std::fs::create_dir_all(&tmp).map_err(|e| { AppError::User(format!( "failed to create temporary profile directory {}: {e}", tmp.display() )) })?; std::fs::write(tmp.join("user.js"), USER_JS).map_err(|e| { AppError::User(format!( "failed to write user.js to temporary profile {}: {e}", tmp.display() )) })?; Ok(tmp) }Then both branches can call
create_temp_profile_dir().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/launch.rs` around lines 246 - 271, Extract the duplicated temp-profile creation logic into a helper function (e.g., create_temp_profile_dir) and call it from both the temp_profile branch and the final else branch that currently builds `tmp`, creates the directory, writes `USER_JS`, and sets `--profile` on `cmd`; the helper should return a Result<PathBuf, AppError>, perform the nonce/timestamp and pid naming, create the directory, write `USER_JS`, and let callers set `cmd.arg("--profile").arg(&tmp)` and handle the returned PathBuf (or propagate the AppError).
🤖 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/dom_tree.rs`:
- Around line 109-156: The truncation of attribute value in render_dom_node
currently slices by byte index (&value[..37]) which can panic on multi-byte
UTF-8; replace the byte-slice with a safe Unicode-aware truncate (e.g., build a
truncated string via value.chars().take(37).collect::<String>() and append "..."
when original value.len() in chars() > 37, or use the unicode-truncate crate)
and update the local variable `val` assignment where `value` is processed so all
attribute truncation is UTF-8 safe.
In `@kb/dogfooding/dogfooding-session-38.md`:
- Around line 27-31: The fenced code block containing the Firefox prefs lines
(user_pref("devtools.debugger.remote-enabled", true);
user_pref("devtools.debugger.prompt-connection", false);
user_pref("devtools.chrome.enabled", true);) lacks a language identifier; update
the opening fence to include a language (e.g., js) so the block becomes ```js to
satisfy MD040 and improve rendering.
---
Nitpick comments:
In `@crates/ff-rdp-cli/src/commands/launch.rs`:
- Around line 246-271: Extract the duplicated temp-profile creation logic into a
helper function (e.g., create_temp_profile_dir) and call it from both the
temp_profile branch and the final else branch that currently builds `tmp`,
creates the directory, writes `USER_JS`, and sets `--profile` on `cmd`; the
helper should return a Result<PathBuf, AppError>, perform the nonce/timestamp
and pid naming, create the directory, write `USER_JS`, and let callers set
`cmd.arg("--profile").arg(&tmp)` and handle the returned PathBuf (or propagate
the AppError).
🪄 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: ddf17df2-90a5-490c-be66-d6dceec8d4d2
📒 Files selected for processing (12)
crates/ff-rdp-cli/src/cli/args.rscrates/ff-rdp-cli/src/commands/dom_tree.rscrates/ff-rdp-cli/src/commands/geometry.rscrates/ff-rdp-cli/src/commands/launch.rscrates/ff-rdp-cli/src/commands/network.rscrates/ff-rdp-cli/src/commands/responsive.rscrates/ff-rdp-cli/src/commands/scroll.rscrates/ff-rdp-cli/tests/e2e/main.rscrates/ff-rdp-cli/tests/e2e/responsive.rscrates/ff-rdp-cli/tests/e2e/static_commands.rskb/dogfooding/dogfooding-session-38.mdkb/iterations/iteration-47-dogfood-bugfixes.md
There was a problem hiding this comment.
Pull request overview
This PR addresses several dogfooding-session bugs by improving first-run launch behavior, fixing scroll position staleness, adding missing --format text outputs, stabilizing responsive layout measurement, and reducing irrelevant flags shown in help for static-output commands.
Changes:
- Auto-create a temporary Firefox profile (with required devtools prefs) when
ff-rdp launchis run without--profile/--temp-profile. - Fix
scrollcommands’ stale viewport readings by deferring position reads viarequestAnimationFrame. - Add/extend
--format textsupport (geometry table, network summary text, DOM tree text) and stabilize responsive geometry via a layout-wait JS snippet; add e2e coverage for static-help output.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-47-dogfood-bugfixes.md | Adds iteration writeup and acceptance criteria for the dogfood bugfix set. |
| kb/dogfooding/dogfooding-session-38.md | Adds session notes that motivated the fixes and documents observed issues. |
| crates/ff-rdp-cli/tests/e2e/static_commands.rs | Adds e2e tests asserting recipes/llm-help --help omit irrelevant global flags. |
| crates/ff-rdp-cli/tests/e2e/responsive.rs | Updates responsive test server expectations to include a per-width layout-wait eval call. |
| crates/ff-rdp-cli/tests/e2e/main.rs | Registers the new static_commands e2e module. |
| crates/ff-rdp-cli/src/commands/scroll.rs | Defers post-scroll viewport reads with requestAnimationFrame to avoid stale positions. |
| crates/ff-rdp-cli/src/commands/responsive.rs | Replaces fixed sleep with JS-based layout stabilization before geometry capture. |
| crates/ff-rdp-cli/src/commands/network.rs | Adds a text-rendered network summary path for --format text in summary mode. |
| crates/ff-rdp-cli/src/commands/launch.rs | Auto-creates a temp profile with devtools prefs when no profile flag is provided. |
| crates/ff-rdp-cli/src/commands/geometry.rs | Adds a text-rendered geometry table path for --format text. |
| crates/ff-rdp-cli/src/commands/dom_tree.rs | Adds an indented DOM tree text renderer for --format text. |
| crates/ff-rdp-cli/src/cli/args.rs | Uses custom clap help_template for recipes and llm-help to reduce irrelevant flag output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+33
| ### 1. Fix `ff-rdp launch` to write remote debugging prefs [0/3] | ||
|
|
||
| `ff-rdp launch --headless --port 6000` fails to connect on a fresh profile because Firefox's remote debugging prefs aren't set. Users must manually create a `user.js` with `devtools.debugger.remote-enabled`, `devtools.debugger.prompt-connection`, and `devtools.chrome.enabled`. The `launch` command should handle this automatically. | ||
|
|
||
| - [x] When creating/using a profile, write the required `user.js` prefs (`devtools.debugger.remote-enabled=true`, `devtools.debugger.prompt-connection=false`, `devtools.chrome.enabled=true`) before launching Firefox | ||
| - [x] Only write prefs if the profile doesn't already have them (avoid clobbering user customizations) | ||
| - [x] Add e2e test that verifies `launch` creates a connectable Firefox instance with a fresh temp profile |
Comment on lines
+124
to
+125
| let val = if value.len() > 40 { | ||
| format!("{}...", &value[..37]) |
Comment on lines
+111
to
+113
| let tag = node_name.to_lowercase(); | ||
| let mut line = format!("{indent}<{tag}"); | ||
|
|
Comment on lines
+155
to
+159
| // Text short-circuit: render a human-readable table instead of JSON. | ||
| if cli.format == "text" && cli.jq.is_none() { | ||
| render_geometry_text(&results); | ||
| return Ok(()); | ||
| } |
…xt path - Fix potential panic in dom_tree text renderer: byte-slice truncation (&value[..37]) replaced with char-based truncation to handle multi-byte UTF-8 safely (found by all three reviewers) - Add missing closing `>` on element tags in dom_tree text output - Fix geometry --format text not applying to zero-match early return path - Update iteration-47 task progress headers to match completed checkboxes - Add language identifier to fenced code block in dogfooding session notes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
This was referenced May 6, 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
--profile/--temp-profilegiven, soff-rdp launch --headless --port 6000works on first use without manualuser.jssetuprequestAnimationFrameto defer position reads afterscrollTo/scrollBy/scrollIntoView, fixing staleviewport.yreporting (e.g.scroll bottomreportingy: 0)--format textforgeometry(tabular with overlaps),networksummary (sections: totals, by-cause-type, slowest), anddom tree(indented tree matchingsnapshot --format textstyle)requestAnimationFrame+setTimeout(0)) to prevent implausible negativerect.yon complex pages--host,--port,--timeout, etc.) from--helpoutput via customhelp_templateTest plan
cargo fmt— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace -q— 693 tests passrequestAnimationFrameandPromise--host/--port/--timeouthidden fromrecipes --helpandllm-help --help🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--format text)ff-rdp launchnow auto-creates a Firefox profile when none is specifiedBug Fixes
Documentation