Conversation
Close the gap between "user runs first command" and "first successful page interaction" by surfacing the diagnostic blind spots the iter-40 dogfooding session hit head-on. - launch detects port collisions before spawning Firefox and aborts with the conflicting PID + a hint pointing at `ff-rdp doctor` and `--port N+10` - new `ff-rdp doctor` subcommand probes daemon, port owner, RDP handshake, tabs, and Firefox version; emits a structured JSON envelope plus ✓/✗/⚠ checklist; exits 0/1 for CI - meta.connection embedded on every browser-touching command's envelope: host, port, connected_pid, uptime_s, firefox_version - "no tabs" error branches by root cause (no listener / connected with zero tabs / connected with un-debuggable tabs), always names doctor - doctor promoted via post-launch hint, --help troubleshooting block, README first-contact section, and connection-error hints - known-failure-mode error messages (actor errors, connection refused, port-in-use, version-mismatch) end with a `hint:` line; connection-related ones name doctor first Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (42)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new end-to-end connection diagnostic workflow to ff-rdp, improving onboarding and making common connection failures (especially port collisions and stale/empty-tab sessions) actionable via structured output and consistent hint: guidance.
Changes:
- Add
ff-rdp doctorsubcommand with JSON report + exit code semantics for CI. - Detect debug-port collisions early in
launchand surface PID/process + next-step hints. - Embed
meta.connection(host/port + best-effort PID/uptime/version) into browser-touching command envelopes and upgrade several connection-related error messages/hints.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents doctor and adds an “First contact (for AI agents)” onboarding flow. |
| kb/iterations/iteration-51-onboarding-fixes.md | Marks iteration-51 tasks as completed. |
| crates/ff-rdp-cli/tests/e2e/tabs.rs | Extends e2e assertions for meta.connection and doctor hints in errors. |
| crates/ff-rdp-cli/tests/e2e/main.rs | Registers the new doctor e2e test module. |
| crates/ff-rdp-cli/tests/e2e/launch.rs | Adds e2e coverage for port collision detection and help mentioning doctor. |
| crates/ff-rdp-cli/tests/e2e/doctor.rs | Adds new e2e tests for doctor behavior across core scenarios. |
| crates/ff-rdp-cli/src/tab_target.rs | Adds context-aware tab resolution and improved “no tabs” error branching + hints. |
| crates/ff-rdp-cli/src/port_owner.rs | Introduces OS-level port owner detection (lsof/netstat/tasklist) + quick TCP probe. |
| crates/ff-rdp-cli/src/main.rs | Wires in new connection_meta and port_owner modules. |
| crates/ff-rdp-cli/src/hints.rs | Promotes ff-rdp doctor as the first post-launch hint. |
| crates/ff-rdp-cli/src/error.rs | Appends actionable hint: lines to several protocol error variants. |
| crates/ff-rdp-cli/src/dispatch.rs | Routes the new Doctor subcommand to its implementation. |
| crates/ff-rdp-cli/src/connection_meta.rs | Adds meta.connection builder + cached PID/version helpers. |
| crates/ff-rdp-cli/src/commands/wait.rs | Merges meta.connection into wait command envelopes. |
| crates/ff-rdp-cli/src/commands/type_text.rs | Merges meta.connection into type-text command envelopes. |
| crates/ff-rdp-cli/src/commands/tabs.rs | Adds doctor-first connection hints; records Firefox version for meta caching. |
| crates/ff-rdp-cli/src/commands/styles.rs | Merges meta.connection into styles command envelopes. |
| crates/ff-rdp-cli/src/commands/storage.rs | Merges meta.connection into storage command envelopes. |
| crates/ff-rdp-cli/src/commands/snapshot.rs | Merges meta.connection into snapshot command envelopes. |
| crates/ff-rdp-cli/src/commands/scroll.rs | Merges meta.connection into scroll command envelopes. |
| crates/ff-rdp-cli/src/commands/screenshot.rs | Merges meta.connection into screenshot command envelopes. |
| crates/ff-rdp-cli/src/commands/responsive.rs | Merges meta.connection into responsive command envelopes. |
| crates/ff-rdp-cli/src/commands/perf.rs | Merges meta.connection into perf command envelopes. |
| crates/ff-rdp-cli/src/commands/perf_compare.rs | Merges meta.connection into perf-compare command envelopes. |
| crates/ff-rdp-cli/src/commands/page_text.rs | Merges meta.connection into page-text command envelopes. |
| crates/ff-rdp-cli/src/commands/navigate.rs | Merges meta.connection into navigate command envelopes. |
| crates/ff-rdp-cli/src/commands/nav_action.rs | Merges meta.connection into navigation action command envelopes. |
| crates/ff-rdp-cli/src/commands/mod.rs | Exposes the new doctor command module. |
| crates/ff-rdp-cli/src/commands/launch.rs | Adds pre-spawn port collision detection; merges meta.connection into output. |
| crates/ff-rdp-cli/src/commands/inspect.rs | Merges meta.connection into inspect command envelopes. |
| crates/ff-rdp-cli/src/commands/geometry.rs | Merges meta.connection into geometry command envelopes. |
| crates/ff-rdp-cli/src/commands/eval.rs | Merges meta.connection into eval command envelopes. |
| crates/ff-rdp-cli/src/commands/dom.rs | Merges meta.connection into DOM command envelopes. |
| crates/ff-rdp-cli/src/commands/doctor.rs | Implements the doctor probe pipeline and JSON results rendering. |
| crates/ff-rdp-cli/src/commands/console.rs | Merges meta.connection into console command envelopes. |
| crates/ff-rdp-cli/src/commands/connect_tab.rs | Integrates context-aware tab resolution and remembers version for meta. |
| crates/ff-rdp-cli/src/commands/computed.rs | Merges meta.connection into computed command envelopes. |
| crates/ff-rdp-cli/src/commands/click.rs | Merges meta.connection into click command envelopes. |
| crates/ff-rdp-cli/src/commands/a11y.rs | Merges meta.connection into a11y command envelopes. |
| crates/ff-rdp-cli/src/commands/a11y_summary.rs | Merges meta.connection into a11y-summary command envelopes. |
| crates/ff-rdp-cli/src/commands/a11y_contrast.rs | Merges meta.connection into a11y-contrast command envelopes. |
| crates/ff-rdp-cli/src/cli/args.rs | Adds doctor to help text and introduces a troubleshooting section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+62
to
+72
| // 2. Port owner — looked up before the handshake attempt so we can report | ||
| // PID/uptime even when the listener is not Firefox. | ||
| let owner = port_owner::find_listener(port).ok().flatten(); | ||
| let port_in_use = owner.is_some() || port_owner::is_port_in_use(port); | ||
| probes.push(probe_port_owner(port, port_in_use, owner.as_ref())); | ||
|
|
||
| // Skip the remaining probes when the port is dark; nothing to handshake with. | ||
| let mut firefox_version: Option<u32> = None; | ||
| if port_in_use { | ||
| match RdpConnection::connect(host, port, Duration::from_millis(cli.timeout)) { | ||
| Ok(mut conn) => { |
Comment on lines
+98
to
+105
| let in_use = port_owner::is_port_in_use(port); | ||
| if !in_use { | ||
| return format!( | ||
| "no tabs available — nothing is listening on {host}:{port}. \ | ||
| Use `ff-rdp launch --headless --temp-profile` to start Firefox.\n\ | ||
| hint: run `ff-rdp doctor` for a full diagnostic." | ||
| ); | ||
| } |
Comment on lines
+54
to
+67
| #[cfg(not(target_os = "windows"))] | ||
| fn find_listener_unix(port: u16) -> Result<Option<PortOwner>, String> { | ||
| let output = std::process::Command::new("lsof") | ||
| .arg("-nP") | ||
| .arg(format!("-iTCP:{port}")) | ||
| .arg("-sTCP:LISTEN") | ||
| .arg("-Fpcn") | ||
| .output() | ||
| .map_err(|e| format!("running lsof: {e}"))?; | ||
|
|
||
| if !output.status.success() { | ||
| // `lsof` exits 1 when no rows match — that means no listener. | ||
| return Ok(None); | ||
| } |
Comment on lines
+360
to
+373
| if port_owner::is_port_in_use(port) { | ||
| let owner = port_owner::find_listener(port).ok().flatten(); | ||
| let suggested = port.checked_add(10).unwrap_or(port); | ||
| let detail = match &owner { | ||
| Some(o) if !o.process_name.is_empty() => { | ||
| format!("by {} (PID {})", o.process_name, o.pid) | ||
| } | ||
| Some(o) => format!("by PID {}", o.pid), | ||
| None => "by another process".to_owned(), | ||
| }; | ||
| return Err(AppError::User(format!( | ||
| "port {port} is already in use {detail}. \ | ||
| hint: pass --port {suggested} to use a different port, run `ff-rdp doctor` for a full report, or stop the existing listener." | ||
| ))); |
- Consolidate duplicate format_uptime helpers (Local) - Gate doctor/no_tabs_message local OS probes on loopback host (Copilot) - Robust port suggestion near u16 max in launch port-collision (Copilot) - Skip non-CSV/INFO: lines in Windows tasklist parsing (CodeRabbit) - Fix top-level --help doc comment in launch e2e (CodeRabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Implements iteration 51 — closes the diagnostic blind spot that the iter-40 dogfooding session hit (silent port collision, "no tabs" pointing back at
launchafter the user already launched, no way to ask "what's wrong with my connection").launch: pre-spawn TCP probe +lsof/netstat/tasklistlookup, errors with the conflicting PID and a hint toff-rdp doctor.ff-rdp doctorsubcommand: probes daemon registry, port owner, RDP handshake, tab count, and Firefox version compatibility. Outputs a structured JSON envelope plus a ✓/✗/⚠ checklist (text mode); exits 0/1 for CI.meta.connectioneverywhere: every browser-touching command's envelope now embeds{host, port, connected_pid, connected_process, uptime_s, firefox_version}. PID/uptime resolved via OS port table, version cached from the RDP greeting.doctor.doctorpromotion across 5 surfaces: post-launch hint (first item), error hints, top-level--helptroubleshooting block, README first-contact section for AI agents, and connection-error messages.UnknownActor,WrongState,UnrecognizedPacketType, connection refused, port-in-use) now end with ahint:line; connection-related ones namedoctorfirst.Test plan
cargo fmt,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace -qall pass.launch_detects_port_collision— bind a TCP listener, launch against that port, assert the structured error mentionsalready in useandff-rdp doctor.help_mentions_doctor— top-level--helplistsdoctor.doctor_no_listener_fails_with_launch_hint— no listener → exit 1, JSON envelope has a probe with aff-rdp launchhint,meta.connectionpopulated.doctor_mock_server_passes_handshake_and_tabs— mock RDP server → handshake + tabs probes pass.doctor_zero_tabs_fails_tabs_probe— mock with empty tabs → tabs probe fails with a relaunch hint.tabs_outputs_json_envelopeextended to assertmeta.connection.host/meta.connection.port.tabs_connection_refused_shows_helpful_errorextended to assert the newff-rdp doctorhint line.ff-rdp launch --headless --temp-profile && ff-rdp doctor— all green.nc -l 6000, runff-rdp launch— verify error mentions PID + doctor.🤖 Generated with Claude Code