iter-53: stability fixes (navigate wait, daemon noise, screenshot fallback)#56
iter-53: stability fixes (navigate wait, daemon noise, screenshot fallback)#56
Conversation
…lback)
Three small reliability bugs that combined to make the CLI feel flaky on
first contact:
1. `navigate --wait-text` reproducibly failed on the first navigate after
a fresh launch with `noSuchActor`. The console actor was resolved
before navigation and invalidated when the docshell tore down. Fix:
re-resolve the target via `getTarget` after navigation so wait-text
always polls against a fresh console actor.
2. The daemon printed `warning: registry not found ... connecting
directly` even when the direct fallback succeeded. Fix: defer the
warning into `ConnectionTarget::Direct { deferred_warning }` and only
surface it if the direct connection also fails.
3. `screenshot` failed with the raw Firefox `Unable to load actor module`
stack on some builds, with a misleading "relaunch with --headless"
hint. Fix: detect the module-load failure and surface a clean
one-liner naming `ff-rdp doctor`, including the observed Firefox
version.
E2e tests cover each path (regression, happy/broken daemon paths, and
both legacy + Firefox 149+ screenshot module-load failures).
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 (10)
✨ 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
This PR delivers a set of stability-focused fixes across ff-rdp’s navigation wait logic, daemon connection diagnostics, and screenshot error handling, with accompanying e2e/unit regressions to prevent recurrence.
Changes:
- Re-resolve the tab target after navigation so
navigate --wait-textpolls using a fresh console actor (avoidsnoSuchActoron first navigate). - Defer the daemon “registry not found / connecting directly” warning so it’s only emitted when the direct fallback also fails.
- Detect Firefox’s “Unable to load actor module …/screenshot” failure and surface a clean, actionable version-mismatch message (naming
ff-rdp doctor), plus add tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-53-stability-fixes.md | Marks iteration tasks/acceptance criteria as completed. |
| crates/ff-rdp-cli/tests/e2e/support/mock_server.rs | Adds per-method call counters to support new behavioral assertions in e2e tests. |
| crates/ff-rdp-cli/tests/e2e/screenshot.rs | Adds e2e regressions asserting clean screenshot module-load failure messaging (legacy + two-step). |
| crates/ff-rdp-cli/tests/e2e/navigate.rs | Adds e2e regression asserting getTarget is invoked again post-navigate for --wait-text. |
| crates/ff-rdp-cli/tests/e2e/daemon.rs | Adds e2e coverage for deferred-warning behavior (silent on success, visible on dual-failure). |
| crates/ff-rdp-cli/src/daemon/client.rs | Implements deferred-warning propagation via ConnectionTarget::Direct { deferred_warning }. |
| crates/ff-rdp-cli/src/connection_meta.rs | Exposes the remembered Firefox version accessor for improved user-facing error messages. |
| crates/ff-rdp-cli/src/commands/screenshot.rs | Adds module-load failure detection + canonical version-mismatch message across screenshot paths. |
| crates/ff-rdp-cli/src/commands/navigate.rs | Re-resolves target after navigation before polling JS wait conditions. |
| crates/ff-rdp-cli/src/commands/connect_tab.rs | Prints deferred daemon warning only when the direct fallback connect fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| no_daemon: bool, | ||
| ) -> ConnectionTarget { | ||
| if no_daemon { | ||
| return ConnectionTarget::Direct; | ||
| return ConnectionTarget::Direct { | ||
| deferred_warning: None, | ||
| }; |
| // Display includes both the Firefox `error` code and the `message` text. | ||
| err.to_string().contains("Unable to load actor module") |
Summary
navigate --wait-textnow re-resolves the target viagetTargetafter navigation, so wait-text always polls against a fresh console actor. FixesnoSuchActoron the first navigate after a fresh launch.screenshotnow detects the Firefox-internal "Unable to load actor module" failure and surfaces a clean one-liner namingff-rdp doctor(including the observed Firefox version), instead of the misleading "relaunch with --headless" hint.Test plan
cargo fmtcargo clippy --workspace --all-targets -- -D warningscargo test --workspace -q(full workspace passes)navigate --wait-textcallsgetTarget≥2× (asserts the post-navigate re-resolve)prepareCapturemodule-load failure → clean version-mismatch messageis_actor_module_load_failuredetector🤖 Generated with Claude Code