Add contextual hints: suggest follow-up commands after every command#52
Add contextual hints: suggest follow-up commands after every command#52
Conversation
Modelled on hyalo's hints system. After every command, ff-rdp now suggests "what next?" commands contextual to what just ran (e.g., after navigate → snapshot, console --level error, screenshot). Hints help AI agents discover workflows without trial-and-error, and help humans discover flags they might not know exist. - New hints.rs module with HintSource, HintContext, generate_hints() - 34 command-specific hint generators with context-sensitivity (e.g., console hints differ based on error presence) - --hints / --no-hints global flags; default on for text, off for JSON - --jq suppresses hints entirely (pipeline needs clean data) - JSON envelope always includes "hints" key (empty [] when suppressed) - Text mode renders hints as " -> cmd # description" lines - MAX_HINTS = 5 cap per command - 8 unit tests + 5 e2e tests covering all hint modes - Help text updated: OUTPUT FORMAT and AI AGENT TIPS sections Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 51 seconds. ⌛ 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 (4)
📝 WalkthroughWalkthroughThis pull request introduces a contextual hints feature that suggests follow-up commands based on output from CLI operations. It adds hint generation logic, CLI flag controls ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Command
participant HintContext
participant OutputPipeline
participant HintGenerator
participant JSONOutput
User->>Command: Execute command (e.g., `ff-rdp tabs`)
Command->>HintContext: Create HintContext<br/>(source, selector, flags)
Command->>OutputPipeline: finalize_with_hints(envelope,<br/>hint_ctx)
OutputPipeline->>OutputPipeline: Check hints_mode<br/>(on, off, or json_only)
alt hints_mode is enabled
OutputPipeline->>HintGenerator: generate_hints(&hint_ctx)
HintGenerator->>HintGenerator: Dispatch to source<br/>specific generator
HintGenerator-->>OutputPipeline: Vec<Hint>
OutputPipeline->>JSONOutput: inject_hints(envelope, hints)
JSONOutput-->>OutputPipeline: Updated envelope
end
alt Output format is text
OutputPipeline->>JSONOutput: Render text output
OutputPipeline->>JSONOutput: Print hint lines<br/>(if enabled)
else Output format is JSON
OutputPipeline->>JSONOutput: Serialize envelope<br/>with hints array
end
JSONOutput-->>User: Formatted output<br/>with optional hints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 contextual hints system to ff-rdp CLI output so each command can suggest likely follow-up commands (rendered in text mode as -> lines and included in JSON output as a hints array), controlled via new global hint flags.
Changes:
- Introduces
hints.rs(hint model + per-command hint generators) and wires hint generation intoOutputPipeline. - Adds
--hints/--no-hintsglobal flags with defaults based on output format, plus updated help text. - Adds e2e coverage for hint rendering/suppression combinations (text vs JSON vs
--jq).
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-50-contextual-hints.md | Adds iteration write-up documenting the hints feature and rollout tasks/criteria |
| crates/ff-rdp-cli/tests/e2e/main.rs | Registers the new e2e hints test module |
| crates/ff-rdp-cli/tests/e2e/hints.rs | Adds e2e tests for JSON/text hints and --jq suppression |
| crates/ff-rdp-cli/src/output_pipeline.rs | Adds hints mode selection and hint injection/rendering in the output pipeline |
| crates/ff-rdp-cli/src/output.rs | Adds inject_hints helper to insert hints into the JSON envelope |
| crates/ff-rdp-cli/src/main.rs | Exposes the new hints module |
| crates/ff-rdp-cli/src/hints.rs | Implements hint data model, context, generators, and unit tests |
| crates/ff-rdp-cli/src/commands/wait.rs | Passes HintContext into output finalization for wait |
| crates/ff-rdp-cli/src/commands/type_text.rs | Passes selector-aware HintContext for type |
| crates/ff-rdp-cli/src/commands/tabs.rs | Passes HintContext for tabs |
| crates/ff-rdp-cli/src/commands/styles.rs | Passes selector-aware HintContext for styles subcommands |
| crates/ff-rdp-cli/src/commands/storage.rs | Passes storage-type-aware HintContext for storage |
| crates/ff-rdp-cli/src/commands/sources.rs | Passes HintContext for sources |
| crates/ff-rdp-cli/src/commands/snapshot.rs | Passes HintContext for snapshot |
| crates/ff-rdp-cli/src/commands/screenshot.rs | Passes HintContext for screenshot |
| crates/ff-rdp-cli/src/commands/responsive.rs | Passes HintContext for responsive |
| crates/ff-rdp-cli/src/commands/perf_compare.rs | Passes HintContext for perf compare output |
| crates/ff-rdp-cli/src/commands/perf.rs | Passes appropriate HintContext for perf commands (vitals/summary/audit/etc.) |
| crates/ff-rdp-cli/src/commands/page_text.rs | Passes HintContext for page-text |
| crates/ff-rdp-cli/src/commands/network.rs | Passes detail-aware HintContext for network |
| crates/ff-rdp-cli/src/commands/navigate.rs | Passes HintContext for navigate (incl. with-network paths) |
| crates/ff-rdp-cli/src/commands/nav_action.rs | Passes HintContext for reload/back/forward |
| crates/ff-rdp-cli/src/commands/launch.rs | Passes HintContext for launch |
| crates/ff-rdp-cli/src/commands/inspect.rs | Passes HintContext for inspect |
| crates/ff-rdp-cli/src/commands/geometry.rs | Passes selector-aware HintContext for geometry |
| crates/ff-rdp-cli/src/commands/eval.rs | Passes HintContext for eval |
| crates/ff-rdp-cli/src/commands/dom_tree.rs | Passes HintContext for dom tree |
| crates/ff-rdp-cli/src/commands/dom.rs | Passes selector-aware HintContext for dom + dom stats |
| crates/ff-rdp-cli/src/commands/cookies.rs | Passes HintContext for cookies |
| crates/ff-rdp-cli/src/commands/console.rs | Passes error-aware HintContext for console |
| crates/ff-rdp-cli/src/commands/computed.rs | Passes selector-aware HintContext for computed |
| crates/ff-rdp-cli/src/commands/click.rs | Passes selector-aware HintContext for click |
| crates/ff-rdp-cli/src/commands/a11y_summary.rs | Passes HintContext for a11y summary |
| crates/ff-rdp-cli/src/commands/a11y_contrast.rs | Passes fail-only-aware HintContext for a11y contrast |
| crates/ff-rdp-cli/src/commands/a11y.rs | Passes HintContext for a11y |
| crates/ff-rdp-cli/src/cli/args.rs | Documents hints in help text and adds global --hints/--no-hints flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn inject_hints(envelope: &mut Value, hints: &[Hint]) { | ||
| if let Some(obj) = envelope.as_object_mut() { | ||
| let hints_json: Vec<Value> = hints | ||
| .iter() | ||
| .map(|h| serde_json::to_value(h).unwrap_or(Value::Null)) | ||
| .collect(); | ||
| obj.insert("hints".to_string(), Value::Array(hints_json)); | ||
| } |
There was a problem hiding this comment.
inject_hints silently converts any serialization error into null entries via unwrap_or(Value::Null), which can hide bugs and produce a malformed hints array. Since Hint should be infallibly serializable, prefer an expect/unwrap with a clear message, or change inject_hints to return a Result and propagate the error.
| fn hints_dom(ctx: &HintContext) -> Vec<Hint> { | ||
| let sel = ctx.selector.as_deref().unwrap_or("selector"); | ||
| vec![ | ||
| Hint::new( | ||
| format!("Click on \"{sel}\""), | ||
| format!(r#"ff-rdp click "{sel}""#), | ||
| ), | ||
| Hint::new( | ||
| format!("Inspect styles of \"{sel}\""), | ||
| format!(r#"ff-rdp styles "{sel}" --properties color,display"#), | ||
| ), | ||
| Hint::new( | ||
| format!("Get computed color for \"{sel}\""), | ||
| format!(r#"ff-rdp computed "{sel}" --prop color"#), |
There was a problem hiding this comment.
Several hint generators build commands like ff-rdp click "{sel}" by interpolating the selector into double quotes without escaping. This produces non-copy-pasteable (and sometimes shell-invalid) hints for selectors containing quotes (common for attribute selectors like a[href="..."]). Consider adding a small shell-escaping/quoting helper and using it consistently when embedding user-provided selectors/strings into hint command lines.
| fn hints_dom(ctx: &HintContext) -> Vec<Hint> { | |
| let sel = ctx.selector.as_deref().unwrap_or("selector"); | |
| vec![ | |
| Hint::new( | |
| format!("Click on \"{sel}\""), | |
| format!(r#"ff-rdp click "{sel}""#), | |
| ), | |
| Hint::new( | |
| format!("Inspect styles of \"{sel}\""), | |
| format!(r#"ff-rdp styles "{sel}" --properties color,display"#), | |
| ), | |
| Hint::new( | |
| format!("Get computed color for \"{sel}\""), | |
| format!(r#"ff-rdp computed "{sel}" --prop color"#), | |
| fn shell_quote(arg: &str) -> String { | |
| format!("'{}'", arg.replace('\'', r#"'\''"#)) | |
| } | |
| fn hints_dom(ctx: &HintContext) -> Vec<Hint> { | |
| let sel = ctx.selector.as_deref().unwrap_or("selector"); | |
| let quoted_sel = shell_quote(sel); | |
| vec![ | |
| Hint::new( | |
| format!("Click on \"{sel}\""), | |
| format!("ff-rdp click {quoted_sel}"), | |
| ), | |
| Hint::new( | |
| format!("Inspect styles of \"{sel}\""), | |
| format!("ff-rdp styles {quoted_sel} --properties color,display"), | |
| ), | |
| Hint::new( | |
| format!("Get computed color for \"{sel}\""), | |
| format!("ff-rdp computed {quoted_sel} --prop color"), |
| "Run interactive accessibility check", | ||
| "ff-rdp a11y --interactive", | ||
| ), | ||
| Hint::new("Check colour contrast", "ff-rdp a11y contrast"), |
There was a problem hiding this comment.
Spelling/style is inconsistent with the rest of the CLI help text (“color”): this hint uses “colour”. Consider changing to “color” for consistency.
| Hint::new("Check colour contrast", "ff-rdp a11y contrast"), | |
| Hint::new("Check color contrast", "ff-rdp a11y contrast"), |
| /// `--jq` pipes through jq and must suppress hints entirely — the output is | ||
| /// a raw jq value, not a JSON envelope, and contains no hint lines. | ||
| #[test] | ||
| fn jq_suppresses_hints() { | ||
| let fixture = load_fixture("list_tabs_response.json"); | ||
|
|
||
| let server = MockRdpServer::new().on("listTabs", fixture); | ||
| let port = server.port(); | ||
| let handle = std::thread::spawn(move || server.serve_one()); | ||
|
|
||
| let mut args = base_args(port); | ||
| args.extend([ | ||
| "tabs".to_owned(), | ||
| "--jq".to_owned(), | ||
| ".results | length".to_owned(), | ||
| ]); | ||
|
|
||
| let output = std::process::Command::new(ff_rdp_bin()) | ||
| .args(&args) | ||
| .output() | ||
| .expect("failed to spawn ff-rdp"); | ||
|
|
||
| handle.join().unwrap(); | ||
|
|
||
| assert!( | ||
| output.status.success(), | ||
| "expected success, stderr: {}", | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
|
|
||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| let trimmed = stdout.trim(); | ||
|
|
||
| // jq emits the raw numeric value, not a JSON envelope. | ||
| assert_eq!( | ||
| trimmed, "2", | ||
| "jq output must be the raw count '2'; got: {trimmed}" | ||
| ); | ||
|
|
||
| // No hint lines must appear in jq output. | ||
| assert!( | ||
| !stdout.contains(" -> "), | ||
| "jq output must not contain hint lines; got:\n{stdout}" | ||
| ); | ||
|
|
||
| // The output must not be a JSON object with a hints key. | ||
| if let Ok(json) = serde_json::from_str::<serde_json::Value>(trimmed) { | ||
| assert!( | ||
| json.get("hints").is_none(), | ||
| "jq output must not be an envelope with 'hints'; got: {json}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The jq_suppresses_hints e2e test uses a jq expression that’s unaffected by adding a top-level hints key (.results | length), so it won’t catch regressions where hints are still injected into the jq input. Consider adding an assertion with --jq '.' (or . | keys) to verify the jq input/output does not include a hints field when jq mode is enabled.
| - [x] All commands produce contextual hints when hints are enabled | ||
| - [x] Hints are on by default in `--format text`, off in JSON |
There was a problem hiding this comment.
Acceptance criteria claims all commands produce contextual hints when enabled, but at least the scroll command path still uses OutputPipeline::finalize() without providing a HintContext, so it will emit no hints in text mode. Either wire hint contexts into the remaining commands (e.g., scroll) or soften this acceptance criterion to match actual behavior.
| let mut envelope = envelope.clone(); | ||
|
|
||
| // Generate and inject hints when enabled. | ||
| let hints = if self.hints_mode == HintsMode::On { | ||
| hint_ctx.map(generate_hints).unwrap_or_default() | ||
| } else { | ||
| vec![] | ||
| }; | ||
| output::inject_hints(&mut envelope, &hints); | ||
|
|
There was a problem hiding this comment.
finalize_with_hints injects an empty hints array into the envelope even when --jq is active (because this happens before the jq filter runs). That changes jq-visible data (e.g., . | keys, . | length) and contradicts the stated goal that --jq needs “clean data” / hints are suppressed entirely. Consider skipping inject_hints (and any hint generation) whenever self.jq_filter.is_some() so jq operates on the original envelope.
| /// If a `HintContext` is provided and hints are enabled, generates | ||
| /// contextual hints and injects them into the envelope. | ||
| /// | ||
| /// If a jq filter is set, apply it to the full envelope so that users | ||
| /// can access any field (`.results`, `.total`, `.meta`, `.hints`). | ||
| /// Otherwise pretty-print the envelope as-is (JSON) or render a | ||
| /// human-readable table (text). |
There was a problem hiding this comment.
The doc comment says jq users can access .hints, but the CLI behavior/requirements say --jq should suppress hints entirely. After fixing suppression, this comment should be updated so it doesn’t imply .hints is available under --jq.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/ff-rdp-cli/src/commands/dom_tree.rs (1)
66-77:⚠️ Potential issue | 🟡 MinorText short-circuit bypasses hint rendering.
Same pattern as
a11y_summary: whenformat == "text"andjqis unset,render_dom_tree_textprints and returns beforefinalize_with_hintsruns, so no follow-up hints are shown in text mode — which is the default and the mode where hints are supposed to be on by default per the PR.Consider rendering hints for
HintSource::DomTreeafter the text tree is printed (either via a pipeline helper or by callinggenerate_hints+ a small render step here).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/dom_tree.rs` around lines 66 - 77, The text-path early return in dom_tree (when cli.format == "text" && cli.jq.is_none()) calls render_dom_tree_text(&results) and returns before hint generation, so HintSource::DomTree hints never run; after printing the text tree, invoke the same hint pipeline used elsewhere: construct HintContext::new(HintSource::DomTree), then either call OutputPipeline::from_cli(cli)? and finalize_with_hints(&envelope, Some(&hint_ctx)) or call the underlying generate_hints + a small render step so hints are generated and printed in text mode (use the same envelope/meta/results inputs as in the JSON path to keep behavior consistent).crates/ff-rdp-cli/src/commands/a11y_summary.rs (1)
109-118:⚠️ Potential issue | 🟡 MinorText short-circuit bypasses hint rendering.
When
cli.format == "text"(the default), this early return prints the custom summary and exits beforefinalize_with_hintsruns, so no hint lines are emitted fora11y summaryin text mode — contradicting the PR goal of suggesting follow-up commands after every command (hints default on for--format text). JSON-mode output still flows through the pipeline correctly, but the most common path gets no hints.Consider rendering hints after the custom text output, e.g. by generating and rendering them inline, or by refactoring
OutputPipelineto expose a hints-only render path you can invoke here.💡 Sketch of a fix
// Custom text rendering for a11y summary. if cli.format == "text" && cli.jq.is_none() { render_summary_text(&output_results); + let hint_ctx = HintContext::new(HintSource::A11ySummary); + OutputPipeline::from_cli(cli)?.render_hints_only(&hint_ctx); return Ok(()); } let hint_ctx = HintContext::new(HintSource::A11ySummary); OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope, Some(&hint_ctx)) .map_err(AppError::from)(exact API up to you — the key point is that the text short-circuit needs a hint-rendering hook.)
🤖 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_summary.rs` around lines 109 - 118, The early return after calling render_summary_text(&output_results) skips hint rendering; instead, keep the custom text path but invoke the hint pipeline afterwards: create the HintContext (HintContext::new(HintSource::A11ySummary)) and pass it to the existing OutputPipeline (OutputPipeline::from_cli(cli)?) to render only hints (e.g., call a hints-only API or a new helper like finalize_hints or finalize_with_hints_only using &envelope and Some(&hint_ctx)) so both render_summary_text and the hint output are emitted when cli.format == "text" && cli.jq.is_none(); if needed add a small helper on OutputPipeline to expose a hints-only render path and call it from this branch.crates/ff-rdp-cli/src/commands/cookies.rs (1)
65-81:⚠️ Potential issue | 🟡 MinorEnvelope now has both
hint(string) andhints(array) — consider reconciling.The existing "no cookies found" branch injects a
hintstring field into the envelope (lines 66–76). With this change, the pipeline will additionally inject the new top-levelhintsarray. JSON consumers (including the AI agents the PR is designed for) now see two similarly-named fields with different semantics, which is easy to misread.If this is intentional (keeping the narrative
hintseparate from the structured follow-uphints), consider renaming the string field to something more descriptive likenoteordiagnosticto avoid collision with the new hints system. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/cookies.rs` around lines 65 - 81, The envelope insertion currently adds a top-level "hint" string which will collide with the new structured "hints" array; update the branch that mutates the envelope (the block that checks total == 0 and uses envelope.as_object_mut()) to insert a differently named field such as "note" or "diagnostic" instead of "hint" (keep the same message), so consumers of the envelope, and the downstream pipeline that constructs HintContext::new(HintSource::Cookies) and OutputPipeline::from_cli(...).finalize_with_hints(...), will not see both "hint" and "hints" with different semantics.crates/ff-rdp-cli/src/commands/a11y.rs (1)
96-105:⚠️ Potential issue | 🟡 MinorMultiple text-mode commands silently bypass the hints feature.
Text short-circuits in 10 locations across 8 command files (a11y, snapshot, dom_tree, responsive, network, perf, geometry, a11y_summary) return immediately after calling custom renderers, never invoking the output pipeline where hints are generated and displayed.
Per the output_pipeline configuration (lines 60–73), hints default to
HintsMode::Onfor--format textand are rendered infinalize_with_hints(line 119). However, the custom text renderers used in short-circuits have no hint output capability and return before the pipeline runs, so:
- Users who run these commands with default text format never see hints, despite hints defaulting to ON
- Explicit
--hintsis silently ignored in short-circuit paths- Text-mode behavior is inconsistent across commands (some show hints, some don't)
Either integrate hint rendering into each custom text renderer (e.g., call
render_hints(&hints)after rendering tables) or refactor the short-circuit logic to flow through the pipeline when hints are enabled.🤖 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 96 - 105, The short-circuit in the text path (where render_a11y_text is called) skips the OutputPipeline hint rendering; update the logic so when hints are enabled (use the HintContext/HintSource::A11y or the CLI hints flag/state) you do not return immediately but instead run the pipeline's finalize_with_hints (via OutputPipeline::from_cli(...).finalize_with_hints(&envelope, Some(&hint_ctx))) or, alternately, after render_a11y_text call the same hint render path (e.g., render_hints using the HintContext/hints produced by the pipeline) so that finalize_with_hints or an equivalent hint rendering is always invoked when hints are ON; modify the condition around render_a11y_text to only short-circuit when hints are OFF.crates/ff-rdp-cli/src/commands/geometry.rs (1)
98-111:⚠️ Potential issue | 🟠 MajorText-mode short-circuit skips hint rendering.
When results are null and the user is in text mode, the code prints and returns
Ok(())without going throughOutputPipeline. Since hints default to on in text mode per the PR objectives, this path will never emit the-> cmd # descriptionlines users expect.The same issue exists at lines 163-166 for the non-empty text path. Consider routing text output through the pipeline (or calling a helper that renders hints after
render_geometry_text) so text-mode hints are honored. This pattern also affectsnetwork.rs(summary mode) andperf.rs(run_summary/run_audittext short-circuits).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/geometry.rs` around lines 98 - 111, The text-mode short-circuit in geometry.rs skips hint rendering: when geometry.is_null() the code calls render_geometry_text(&empty) and returns Ok(()) instead of running the OutputPipeline hint path; do not return early — either route text output through OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope, Some(&hint_ctx)) (so hints are emitted consistently) or, if you must keep render_geometry_text, call the same hint-emission helper after render_geometry_text (use HintContext::new(HintSource::Geometry).with_selector(first_sel) and the existing envelope/meta) so the -> cmd # description lines are produced; update the matching non-empty text path (the other short-circuit) similarly and mirror the same fix pattern used for network.rs and perf.rs.
♻️ Duplicate comments (3)
crates/ff-rdp-cli/src/commands/perf.rs (2)
1014-1027:⚠️ Potential issue | 🟠 Major
perf audittext mode bypasses hint rendering.Same issue as
perf summaryabove —render_audit_textis invoked and the function returnsOk(())without ever going throughOutputPipeline, so hints never appear in text mode.🤖 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 1014 - 1027, The text-mode short-circuit in perf audit skips hint rendering by calling render_audit_text(&results) and returning early; instead, after producing the text output you must still run the OutputPipeline hint stage. Change the branch so that render_audit_text(&results) is called but do not return immediately — create HintContext::new(HintSource::PerfAudit) and call OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope, Some(&hint_ctx)) (or call finalize_with_hints after render_audit_text), propagating errors via map_err(AppError::from), so hints are emitted in text mode just like other formats.
660-673:⚠️ Potential issue | 🟠 Major
perf summarytext mode bypasses hint rendering.Same pattern as
geometry.rs: thecli.format == "text"branch returns beforefinalize_with_hintsruns, so text-mode users never see the configured hints forperf summary. See root-cause comment ongeometry.rs.🤖 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 660 - 673, The text-mode early return in perf summary skips hint rendering; remove the early return and ensure finalize_with_hints is always invoked. Build meta/envelope and create hint_ctx (HintContext::new(HintSource::PerfSummary)) as shown, call OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope, Some(&hint_ctx)).map_err(AppError::from) before exiting, and only after that, if cli.format == "text" && cli.jq.is_none(), call render_summary_text(&results) and then return Ok(()); keep references to render_summary_text, OutputPipeline::from_cli, finalize_with_hints, HintContext, HintSource::PerfSummary, envelope and meta to locate the change.crates/ff-rdp-cli/src/commands/network.rs (1)
181-196:⚠️ Potential issue | 🟠 MajorSummary-mode text short-circuit bypasses hint rendering.
Line 182-185 returns
Ok(())before reachingfinalize_with_hints, so hints are never printed fornetworksummary in text mode even though text-mode hints default to on. See the root-cause comment ongeometry.rs— the same gap affectsnetworksummary andperf summary/perf audittext paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/network.rs` around lines 181 - 196, The short-circuit return after render_network_summary_text skips building the envelope and calling OutputPipeline::finalize_with_hints, so text-mode hints never render; instead, either build the envelope before the text short-circuit or move render_network_summary_text to run then continue to construct `envelope` and call `OutputPipeline::finalize_with_hints(&envelope, Some(&hint_ctx))`; ensure `empty_hint` is injected into the same `envelope` (using `envelope.as_object_mut()`), and keep `HintContext::new(HintSource::Network).with_detail(cli.detail)` so hints are emitted for text summary paths without an early `return Ok(())`.
🧹 Nitpick comments (8)
crates/ff-rdp-cli/src/commands/sources.rs (1)
130-133: Hints plumbing is consistent with the rest of the CLI. Optionally, since this command setsfallback/fallback_methodin meta when JS fallback was used, you could consider whetherHintContextshould carry that signal so the generator can suggest e.g. upgrading Firefox or using the native path — but this is clearly optional and can be deferred.🤖 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 130 - 133, The hint pipeline is created with HintContext::new(HintSource::Sources) but doesn't convey that this command may set fallback/fallback_method in the envelope meta; update the HintContext to carry a fallback flag (or enum) and set it here so finalize_with_hints(&envelope, Some(&hint_ctx)) can generate fallback-specific suggestions. Modify HintContext (e.g., add a field like fallback_used or fallback_method) and its constructor used here (HintContext::new) to accept that signal, populate it when JS fallback was used (the code path that sets envelope.meta.fallback/fallback_method), and ensure finalize_with_hints reads that field to tailor hints (references: HintContext, HintSource::Sources, finalize_with_hints, envelope, fallback/fallback_method).crates/ff-rdp-cli/src/commands/inspect.rs (1)
41-44: Hints plumbing consistent with sibling commands. Optionally, if inspect-specific hint generators ever want to suggest follow-ups that reference the inspected actor, you could extendHintContext(e.g. anwith_actorbuilder) — purely optional, defer if hint generators don't need it today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/inspect.rs` around lines 41 - 44, The inspect command currently creates a HintContext via HintContext::new(HintSource::Inspect) and passes it to OutputPipeline::finalize_with_hints(&envelope, Some(&hint_ctx)); to allow inspect-specific hint generators to reference the inspected actor, add a builder method on HintContext (e.g., HintContext::with_actor or a with_actor(self, actor_id: ActorId) -> Self) and use it here to attach the inspected actor before calling finalize_with_hints so hint generators can access that actor context when producing follow-up suggestions.crates/ff-rdp-cli/src/commands/storage.rs (1)
34-102: Hoist theHintContextconstruction to avoid repeating it in all three branches.
canonical_typeis fixed at the top ofrun, and the hint context is identical in theGrip::Null, value, and all-keys dump branches. Constructing it once keeps the three finalization sites parallel and makes it harder to let them drift (e.g., if someone later adds.with_url(...)in only one branch).♻️ Suggested refactor
let mut ctx = connect_direct(cli)?; let console_actor = ctx.target.console_actor.clone(); let meta = json!({ "host": cli.host, "port": cli.port, "storage_type": canonical_type, }); + let hint_ctx = HintContext::new(HintSource::Storage).with_storage_type(canonical_type); + if let Some(k) = key { // Single-key lookup: embed key as a JSON-encoded string literal to // prevent any injection through the key name. let key_json = serde_json::to_string(k) .map_err(|e| AppError::Internal(anyhow::anyhow!("key serialisation: {e}")))?; ... match &eval_result.result { Grip::Null => { let envelope = output::envelope(&json!({"key": k, "value": null}), 0, &meta); - let hint_ctx = - HintContext::new(HintSource::Storage).with_storage_type(canonical_type); OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope, Some(&hint_ctx)) .map_err(AppError::from) } grip => { let value = resolve_string_grip(&mut ctx, grip)?; let envelope = output::envelope(&json!({"key": k, "value": value}), 1, &meta); - let hint_ctx = - HintContext::new(HintSource::Storage).with_storage_type(canonical_type); OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope, Some(&hint_ctx)) .map_err(AppError::from) } } } else { ... let envelope = output::envelope(&storage_map, total, &meta); - let hint_ctx = HintContext::new(HintSource::Storage).with_storage_type(canonical_type); OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope, Some(&hint_ctx)) .map_err(AppError::from) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/storage.rs` around lines 34 - 102, The HintContext construction is duplicated across the Grip::Null, value, and all-keys dump branches; create it once before the if/else branch by calling HintContext::new(HintSource::Storage).with_storage_type(canonical_type) (use the same canonical_type already computed) and then pass that single hint_ctx into each OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope, Some(&hint_ctx)). This removes repetition around HintContext and ensures all finalization calls use the identical context.crates/ff-rdp-cli/src/commands/navigate.rs (1)
82-86: Hint context wiring is consistent across all three output paths — LGTM.
HintSource::Navigateis applied uniformly to the daemon, direct, and no-network branches, so navigate output is labeled consistently regardless of which code path runs.Optional: since
HintContext::new(HintSource::Navigate)is constructed verbatim three times, you could hoist a tiny helper (fn nav_hint_ctx() -> HintContext) if you anticipate adding more fields (e.g.,.with_url(url)) later. Not worth doing now if the context stays trivial.Also applies to: 214-217, 289-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/navigate.rs` around lines 82 - 86, Multiple places construct the same HintContext via HintContext::new(HintSource::Navigate); extract that repeated construction into a small helper function (e.g., fn nav_hint_ctx() -> HintContext) and replace the three inline calls with nav_hint_ctx() so future additions like .with_url(url) can be applied in one place; update the call sites that currently use HintContext::new(HintSource::Navigate) (seen near the OutputPipeline::from_cli(...).finalize_with_hints(&envelope, Some(&hint_ctx)) usages) to use the new helper.crates/ff-rdp-cli/src/commands/perf.rs (1)
1217-1221:run_group_by_domainreusesHintSource::Perf.Minor consistency nit:
run_group_by_domaintags its hints with the genericHintSource::Perf, while other perf sub-commands have dedicated variants (PerfVitals,PerfSummary,PerfAudit). If the intent is identical hints toperf <type>, fine; otherwise consider adding a dedicated variant (or at least documenting the reuse) so future hint tweaks can target grouped output independently.🤖 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 1217 - 1221, run_group_by_domain currently tags hints with the generic HintSource::Perf which prevents per-subcommand differentiation; add a dedicated enum variant (e.g. PerfGroupByDomain) to HintSource, update its serde/match arms/usages, and replace the HintSource::Perf in run_group_by_domain with the new variant (or alternatively add a clear comment in the run_group_by_domain function explaining intentional reuse) so future hint handling can target this subcommand independently; ensure you update any pattern matches, constructors, and tests that exhaustively match HintSource variants (and bump any derived traits if needed).crates/ff-rdp-cli/src/commands/network.rs (1)
128-196: Coexistinghint(string) andhints(array) fields in the envelope.The existing inline
"hint"message (e.g. "No network events captured…") is now emitted alongside the new top-level"hints": [...]array. JSON consumers will see both keys with different shapes/meanings. Consider documenting this in the schema notes or folding the empty/filter hint into the structured hints list to keep one canonical field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/commands/network.rs` around lines 128 - 196, The code currently inserts a top-level "hint" string (via empty_hint) into the JSON envelope, causing both "hint" and the structured "hints" array to coexist; change the insertion logic in both places that mutate the envelope (after output::envelope_with_truncation and after output::envelope) to instead fold the empty_hint into the structured "hints" array: when envelope.as_object_mut() is Some, if a "hints" array exists push the empty_hint into it, otherwise create a new "hints" array containing the empty_hint; remove the obj.insert("hint", ...) usage and keep the rest (HintContext::new(HintSource::Network), finalize_with_hints, etc.) unchanged so consumers only see the canonical "hints" field.crates/ff-rdp-cli/src/output_pipeline.rs (1)
130-132: Drop the unnecessary turbofish.
Nonealone is sufficient since the parameter typeOption<&HintContext>constrains inference.♻️ Nit
pub fn finalize(&self, envelope: &Value) -> anyhow::Result<()> { - self.finalize_with_hints(envelope, None::<&HintContext>) + self.finalize_with_hints(envelope, None) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/src/output_pipeline.rs` around lines 130 - 132, The finalize method calls finalize_with_hints using the explicit turbofish None::<&HintContext> which is unnecessary because the parameter type Option<&HintContext> already guides type inference; update the finalize function to pass plain None to finalize_with_hints (leave finalize, finalize_with_hints, and HintContext names as-is) to remove the redundant turbofish.crates/ff-rdp-cli/tests/e2e/hints.rs (1)
242-247: Tautological assertion — can be removed or strengthened.At this point
trimmed == "2"has already been asserted, soserde_json::from_str(trimmed)produces aNumber, andjson.get("hints")on a non-object always returnsNone. This block doesn't exercise anything the earlier asserts didn't. Consider dropping it, or instead strengthening the check earlier by also running the command with a jq filter that returns an object (e.g.,--jq .) and asserting no"hints"key appears in that envelope-like output.♻️ Proposed cleanup
- // The output must not be a JSON object with a hints key. - if let Ok(json) = serde_json::from_str::<serde_json::Value>(trimmed) { - assert!( - json.get("hints").is_none(), - "jq output must not be an envelope with 'hints'; got: {json}" - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ff-rdp-cli/tests/e2e/hints.rs` around lines 242 - 247, The extra serde_json check is tautological because earlier assert ensures trimmed == "2", so drop this block or, preferably, strengthen the test by re-running the CLI invocation with a jq envelope (e.g., pass the --jq . option) and then parsing that output: call serde_json::from_str::<serde_json::Value> on the envelope output and assert json.get("hints").is_none() to ensure the tool does not inject a "hints" key into object envelopes; update the test in hints.rs to use the envelope run and an assertion against json.get("hints") instead of the current redundant branch that parses the scalar "2".
🤖 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/console.rs`:
- Around line 126-127: The current lookup for error_count uses a case-sensitive
get on by_level ("let error_count = by_level.get(\"error\")...") but msg.level
keys preserve original casing, so normalize case to avoid misses: either
(preferred) change where by_level is constructed (the aggregation that uses
msg.level) to insert keys lowercased (e.g., use msg.level.to_ascii_lowercase())
so all lookups can be lowercase, or instead change the error_count calculation
to sum counts for keys matching "error" case-insensitively (iterate by_level and
sum values where key.eq_ignore_ascii_case("error")); update the reference to
error_count and any has_errors logic accordingly.
In `@crates/ff-rdp-cli/src/hints.rs`:
- Around line 72-87: The HintContext struct exposes all fields as pub which
expands the public surface; remove the pub visibility from its fields so they
are private and only accessible via the builder API (HintContext::new and the
with_* chain methods), ensuring external callers must use the
constructor/builder and cannot bypass invariants—update any in-module code that
accesses those fields to reference them directly (still allowed) and ensure
tests/examples construct HintContext only through new/with_*.
- Around line 274-283: In hints_type_text, the user-facing description
references the input selector variable sel but the command string always clicks
button[type=submit], causing a mismatch; update the Hint::new pair so
description and cmd align—either change the description to mention submitting
via the submit button (e.g., "Click submit button for the form") or change the
command to use sel (or a form-related selector derived from sel) so the command
and the description both reference the same target; locate the Hint::new calls
inside hints_type_text and make the description and the corresponding command
consistent.
In `@kb/iterations/iteration-50-contextual-hints.md`:
- Around line 36-39: Change the fenced block that currently starts with "```"
and contains the illustrative lines "-> ff-rdp console --level error # Check
for console errors" and "-> ff-rdp screenshot -o page.png # Capture a
screenshot" to specify a language of "text" (i.e., use "```text") so
markdownlint MD040 is satisfied; keep the block contents unchanged and ensure
the closing fence remains "```".
---
Outside diff comments:
In `@crates/ff-rdp-cli/src/commands/a11y_summary.rs`:
- Around line 109-118: The early return after calling
render_summary_text(&output_results) skips hint rendering; instead, keep the
custom text path but invoke the hint pipeline afterwards: create the HintContext
(HintContext::new(HintSource::A11ySummary)) and pass it to the existing
OutputPipeline (OutputPipeline::from_cli(cli)?) to render only hints (e.g., call
a hints-only API or a new helper like finalize_hints or finalize_with_hints_only
using &envelope and Some(&hint_ctx)) so both render_summary_text and the hint
output are emitted when cli.format == "text" && cli.jq.is_none(); if needed add
a small helper on OutputPipeline to expose a hints-only render path and call it
from this branch.
In `@crates/ff-rdp-cli/src/commands/a11y.rs`:
- Around line 96-105: The short-circuit in the text path (where render_a11y_text
is called) skips the OutputPipeline hint rendering; update the logic so when
hints are enabled (use the HintContext/HintSource::A11y or the CLI hints
flag/state) you do not return immediately but instead run the pipeline's
finalize_with_hints (via
OutputPipeline::from_cli(...).finalize_with_hints(&envelope, Some(&hint_ctx)))
or, alternately, after render_a11y_text call the same hint render path (e.g.,
render_hints using the HintContext/hints produced by the pipeline) so that
finalize_with_hints or an equivalent hint rendering is always invoked when hints
are ON; modify the condition around render_a11y_text to only short-circuit when
hints are OFF.
In `@crates/ff-rdp-cli/src/commands/cookies.rs`:
- Around line 65-81: The envelope insertion currently adds a top-level "hint"
string which will collide with the new structured "hints" array; update the
branch that mutates the envelope (the block that checks total == 0 and uses
envelope.as_object_mut()) to insert a differently named field such as "note" or
"diagnostic" instead of "hint" (keep the same message), so consumers of the
envelope, and the downstream pipeline that constructs
HintContext::new(HintSource::Cookies) and
OutputPipeline::from_cli(...).finalize_with_hints(...), will not see both "hint"
and "hints" with different semantics.
In `@crates/ff-rdp-cli/src/commands/dom_tree.rs`:
- Around line 66-77: The text-path early return in dom_tree (when cli.format ==
"text" && cli.jq.is_none()) calls render_dom_tree_text(&results) and returns
before hint generation, so HintSource::DomTree hints never run; after printing
the text tree, invoke the same hint pipeline used elsewhere: construct
HintContext::new(HintSource::DomTree), then either call
OutputPipeline::from_cli(cli)? and finalize_with_hints(&envelope,
Some(&hint_ctx)) or call the underlying generate_hints + a small render step so
hints are generated and printed in text mode (use the same envelope/meta/results
inputs as in the JSON path to keep behavior consistent).
In `@crates/ff-rdp-cli/src/commands/geometry.rs`:
- Around line 98-111: The text-mode short-circuit in geometry.rs skips hint
rendering: when geometry.is_null() the code calls render_geometry_text(&empty)
and returns Ok(()) instead of running the OutputPipeline hint path; do not
return early — either route text output through
OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope, Some(&hint_ctx))
(so hints are emitted consistently) or, if you must keep render_geometry_text,
call the same hint-emission helper after render_geometry_text (use
HintContext::new(HintSource::Geometry).with_selector(first_sel) and the existing
envelope/meta) so the -> cmd # description lines are produced; update the
matching non-empty text path (the other short-circuit) similarly and mirror the
same fix pattern used for network.rs and perf.rs.
---
Duplicate comments:
In `@crates/ff-rdp-cli/src/commands/network.rs`:
- Around line 181-196: The short-circuit return after
render_network_summary_text skips building the envelope and calling
OutputPipeline::finalize_with_hints, so text-mode hints never render; instead,
either build the envelope before the text short-circuit or move
render_network_summary_text to run then continue to construct `envelope` and
call `OutputPipeline::finalize_with_hints(&envelope, Some(&hint_ctx))`; ensure
`empty_hint` is injected into the same `envelope` (using
`envelope.as_object_mut()`), and keep
`HintContext::new(HintSource::Network).with_detail(cli.detail)` so hints are
emitted for text summary paths without an early `return Ok(())`.
In `@crates/ff-rdp-cli/src/commands/perf.rs`:
- Around line 1014-1027: The text-mode short-circuit in perf audit skips hint
rendering by calling render_audit_text(&results) and returning early; instead,
after producing the text output you must still run the OutputPipeline hint
stage. Change the branch so that render_audit_text(&results) is called but do
not return immediately — create HintContext::new(HintSource::PerfAudit) and call
OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope, Some(&hint_ctx))
(or call finalize_with_hints after render_audit_text), propagating errors via
map_err(AppError::from), so hints are emitted in text mode just like other
formats.
- Around line 660-673: The text-mode early return in perf summary skips hint
rendering; remove the early return and ensure finalize_with_hints is always
invoked. Build meta/envelope and create hint_ctx
(HintContext::new(HintSource::PerfSummary)) as shown, call
OutputPipeline::from_cli(cli)? .finalize_with_hints(&envelope,
Some(&hint_ctx)).map_err(AppError::from) before exiting, and only after that, if
cli.format == "text" && cli.jq.is_none(), call render_summary_text(&results) and
then return Ok(()); keep references to render_summary_text,
OutputPipeline::from_cli, finalize_with_hints, HintContext,
HintSource::PerfSummary, envelope and meta to locate the change.
---
Nitpick comments:
In `@crates/ff-rdp-cli/src/commands/inspect.rs`:
- Around line 41-44: The inspect command currently creates a HintContext via
HintContext::new(HintSource::Inspect) and passes it to
OutputPipeline::finalize_with_hints(&envelope, Some(&hint_ctx)); to allow
inspect-specific hint generators to reference the inspected actor, add a builder
method on HintContext (e.g., HintContext::with_actor or a with_actor(self,
actor_id: ActorId) -> Self) and use it here to attach the inspected actor before
calling finalize_with_hints so hint generators can access that actor context
when producing follow-up suggestions.
In `@crates/ff-rdp-cli/src/commands/navigate.rs`:
- Around line 82-86: Multiple places construct the same HintContext via
HintContext::new(HintSource::Navigate); extract that repeated construction into
a small helper function (e.g., fn nav_hint_ctx() -> HintContext) and replace the
three inline calls with nav_hint_ctx() so future additions like .with_url(url)
can be applied in one place; update the call sites that currently use
HintContext::new(HintSource::Navigate) (seen near the
OutputPipeline::from_cli(...).finalize_with_hints(&envelope, Some(&hint_ctx))
usages) to use the new helper.
In `@crates/ff-rdp-cli/src/commands/network.rs`:
- Around line 128-196: The code currently inserts a top-level "hint" string (via
empty_hint) into the JSON envelope, causing both "hint" and the structured
"hints" array to coexist; change the insertion logic in both places that mutate
the envelope (after output::envelope_with_truncation and after output::envelope)
to instead fold the empty_hint into the structured "hints" array: when
envelope.as_object_mut() is Some, if a "hints" array exists push the empty_hint
into it, otherwise create a new "hints" array containing the empty_hint; remove
the obj.insert("hint", ...) usage and keep the rest
(HintContext::new(HintSource::Network), finalize_with_hints, etc.) unchanged so
consumers only see the canonical "hints" field.
In `@crates/ff-rdp-cli/src/commands/perf.rs`:
- Around line 1217-1221: run_group_by_domain currently tags hints with the
generic HintSource::Perf which prevents per-subcommand differentiation; add a
dedicated enum variant (e.g. PerfGroupByDomain) to HintSource, update its
serde/match arms/usages, and replace the HintSource::Perf in run_group_by_domain
with the new variant (or alternatively add a clear comment in the
run_group_by_domain function explaining intentional reuse) so future hint
handling can target this subcommand independently; ensure you update any pattern
matches, constructors, and tests that exhaustively match HintSource variants
(and bump any derived traits if needed).
In `@crates/ff-rdp-cli/src/commands/sources.rs`:
- Around line 130-133: The hint pipeline is created with
HintContext::new(HintSource::Sources) but doesn't convey that this command may
set fallback/fallback_method in the envelope meta; update the HintContext to
carry a fallback flag (or enum) and set it here so
finalize_with_hints(&envelope, Some(&hint_ctx)) can generate fallback-specific
suggestions. Modify HintContext (e.g., add a field like fallback_used or
fallback_method) and its constructor used here (HintContext::new) to accept that
signal, populate it when JS fallback was used (the code path that sets
envelope.meta.fallback/fallback_method), and ensure finalize_with_hints reads
that field to tailor hints (references: HintContext, HintSource::Sources,
finalize_with_hints, envelope, fallback/fallback_method).
In `@crates/ff-rdp-cli/src/commands/storage.rs`:
- Around line 34-102: The HintContext construction is duplicated across the
Grip::Null, value, and all-keys dump branches; create it once before the if/else
branch by calling
HintContext::new(HintSource::Storage).with_storage_type(canonical_type) (use the
same canonical_type already computed) and then pass that single hint_ctx into
each OutputPipeline::from_cli(cli)?.finalize_with_hints(&envelope,
Some(&hint_ctx)). This removes repetition around HintContext and ensures all
finalization calls use the identical context.
In `@crates/ff-rdp-cli/src/output_pipeline.rs`:
- Around line 130-132: The finalize method calls finalize_with_hints using the
explicit turbofish None::<&HintContext> which is unnecessary because the
parameter type Option<&HintContext> already guides type inference; update the
finalize function to pass plain None to finalize_with_hints (leave finalize,
finalize_with_hints, and HintContext names as-is) to remove the redundant
turbofish.
In `@crates/ff-rdp-cli/tests/e2e/hints.rs`:
- Around line 242-247: The extra serde_json check is tautological because
earlier assert ensures trimmed == "2", so drop this block or, preferably,
strengthen the test by re-running the CLI invocation with a jq envelope (e.g.,
pass the --jq . option) and then parsing that output: call
serde_json::from_str::<serde_json::Value> on the envelope output and assert
json.get("hints").is_none() to ensure the tool does not inject a "hints" key
into object envelopes; update the test in hints.rs to use the envelope run and
an assertion against json.get("hints") instead of the current redundant branch
that parses the scalar "2".
🪄 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: 2158da91-e327-4d21-a4c0-e86eb9b7ea17
📒 Files selected for processing (36)
crates/ff-rdp-cli/src/cli/args.rscrates/ff-rdp-cli/src/commands/a11y.rscrates/ff-rdp-cli/src/commands/a11y_contrast.rscrates/ff-rdp-cli/src/commands/a11y_summary.rscrates/ff-rdp-cli/src/commands/click.rscrates/ff-rdp-cli/src/commands/computed.rscrates/ff-rdp-cli/src/commands/console.rscrates/ff-rdp-cli/src/commands/cookies.rscrates/ff-rdp-cli/src/commands/dom.rscrates/ff-rdp-cli/src/commands/dom_tree.rscrates/ff-rdp-cli/src/commands/eval.rscrates/ff-rdp-cli/src/commands/geometry.rscrates/ff-rdp-cli/src/commands/inspect.rscrates/ff-rdp-cli/src/commands/launch.rscrates/ff-rdp-cli/src/commands/nav_action.rscrates/ff-rdp-cli/src/commands/navigate.rscrates/ff-rdp-cli/src/commands/network.rscrates/ff-rdp-cli/src/commands/page_text.rscrates/ff-rdp-cli/src/commands/perf.rscrates/ff-rdp-cli/src/commands/perf_compare.rscrates/ff-rdp-cli/src/commands/responsive.rscrates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/commands/snapshot.rscrates/ff-rdp-cli/src/commands/sources.rscrates/ff-rdp-cli/src/commands/storage.rscrates/ff-rdp-cli/src/commands/styles.rscrates/ff-rdp-cli/src/commands/tabs.rscrates/ff-rdp-cli/src/commands/type_text.rscrates/ff-rdp-cli/src/commands/wait.rscrates/ff-rdp-cli/src/hints.rscrates/ff-rdp-cli/src/main.rscrates/ff-rdp-cli/src/output.rscrates/ff-rdp-cli/src/output_pipeline.rscrates/ff-rdp-cli/tests/e2e/hints.rscrates/ff-rdp-cli/tests/e2e/main.rskb/iterations/iteration-50-contextual-hints.md
| // Capture error count before consuming by_level. | ||
| let error_count = by_level.get("error").copied().unwrap_or(0); |
There was a problem hiding this comment.
Case-sensitive lookup on by_level may miss non-lowercase error levels.
by_level keys preserve the original case from msg.level (line 84), while other comparisons in this file use eq_ignore_ascii_case. Firefox's RDP reliably emits lowercase level strings today, but if that ever changes (e.g. "Error") error_count silently drops to 0 and the has_errors hint branch is skipped. Consider normalizing to lowercase when building the key, or summing over case-insensitive matches here.
Proposed defensive fix
- let error_count = by_level.get("error").copied().unwrap_or(0);
+ let error_count: usize = by_level
+ .iter()
+ .filter(|(k, _)| k.eq_ignore_ascii_case("error"))
+ .map(|(_, v)| *v)
+ .sum();📝 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.
| // Capture error count before consuming by_level. | |
| let error_count = by_level.get("error").copied().unwrap_or(0); | |
| // Capture error count before consuming by_level. | |
| let error_count: usize = by_level | |
| .iter() | |
| .filter(|(k, _)| k.eq_ignore_ascii_case("error")) | |
| .map(|(_, v)| *v) | |
| .sum(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ff-rdp-cli/src/commands/console.rs` around lines 126 - 127, The
current lookup for error_count uses a case-sensitive get on by_level ("let
error_count = by_level.get(\"error\")...") but msg.level keys preserve original
casing, so normalize case to avoid misses: either (preferred) change where
by_level is constructed (the aggregation that uses msg.level) to insert keys
lowercased (e.g., use msg.level.to_ascii_lowercase()) so all lookups can be
lowercase, or instead change the error_count calculation to sum counts for keys
matching "error" case-insensitively (iterate by_level and sum values where
key.eq_ignore_ascii_case("error")); update the reference to error_count and any
has_errors logic accordingly.
| ``` | ||
| -> ff-rdp console --level error # Check for console errors | ||
| -> ff-rdp screenshot -o page.png # Capture a screenshot | ||
| ``` |
There was a problem hiding this comment.
Add a language to the text-rendering fenced block.
markdownlint flags MD040 on this block. Since this is illustrative output rather than code, text is appropriate.
Proposed fix
-```
+```text
-> ff-rdp console --level error # Check for console errors
-> ff-rdp screenshot -o page.png # Capture a screenshot</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kb/iterations/iteration-50-contextual-hints.md` around lines 36 - 39, Change
the fenced block that currently starts with "```" and contains the illustrative
lines "-> ff-rdp console --level error # Check for console errors" and "->
ff-rdp screenshot -o page.png # Capture a screenshot" to specify a language of
"text" (i.e., use "```text") so markdownlint MD040 is satisfied; keep the block
contents unchanged and ensure the closing fence remains "```".
…uppression - Propagate serialization error in inject_hints instead of swallowing with unwrap_or - Gate hint injection: don't inject empty hints array when HintsMode::Off - Escape selectors for shell safety in hint commands - Restrict pub fields to pub(crate) on Hint and HintContext - Remove dead url field from HintContext - Fix "colour" → "color" spelling, fix misleading type_text description - Update doc comments and e2e test for hints-off behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
navigate, hints suggestsnapshot --depth 3,console --level error,screenshot -o page.pnghints.rsmodule with 34 command-specific hint generators, many context-sensitive (console hints differ on error presence, a11y contrast differs on--fail-only, storage hints suggest the other storage type)--hints/--no-hintsglobal flags; default on for--format text, off for JSON;--jqalways suppresses hints"hints"key (empty[]when suppressed); text mode renders as-> cmd # descriptionlinesTest plan
cargo fmt— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace -q— 726 tests pass (including 5 new e2e hint tests)ff-rdp tabs --format textshows->hint linesff-rdp tabsJSON output has"hints": []ff-rdp tabs --hintsJSON output has populated hints arrayff-rdp tabs --jq '.total'suppresses hints entirely🤖 Generated with Claude Code
Summary by CodeRabbit
--hintsand--no-hintsflags to control contextual command suggestions."hints"field.--jqfor clean pipeline data.