Skip to content

Fix scroll viewport and reload --wait-idle in daemon mode#51

Merged
ractive merged 2 commits intomainfrom
iter-49/scroll-reload-fixes
Apr 23, 2026
Merged

Fix scroll viewport and reload --wait-idle in daemon mode#51
ractive merged 2 commits intomainfrom
iter-49/scroll-reload-fixes

Conversation

@ractive
Copy link
Copy Markdown
Owner

@ractive ractive commented Apr 17, 2026

Summary

  • scroll: Remove Promise+requestAnimationFrame wrappers from all scroll commands (top, bottom, by, to, container). Firefox's evaluateJSAsync returns immediately without waiting for Promise resolution, so the commands returned a Promise actor grip ({"class": "Promise"}) instead of actual viewport position data. Now reads viewport position synchronously after scroll calls.
  • reload --wait-idle: Add daemon-aware branching. In daemon mode, the daemon intercepts watcher events and buffers them — they never reach the reload client's recv() loop. Now uses start_daemon_stream/stop_daemon_stream_draining (same pattern as navigate --with-network) so events are forwarded directly.
  • Adds dogfooding session 39 report (GitHub target, verified iter-47/48 fixes, found these two remaining bugs).

Test plan

  • cargo fmt — clean
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo test --workspace -q — 710 tests pass
  • Manual test: scroll bottomviewport.y: 2940 (was Promise grip)
  • Manual test: scroll topviewport.y: 0 (correct)
  • Manual test: scroll by --dy 500viewport.y: 500 (correct)
  • Manual test: reload --wait-idlerequests_observed: 538, idle_at_ms: 1129 (was 0 requests / 10s timeout)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • reload --wait-idle now reliably counts network requests in both daemon and direct modes
    • Scroll commands now return geometry results synchronously instead of Promise objects
  • Documentation

    • Added a new dogfooding session report summarizing recent tests, regressions, and findings
  • Tests

    • Improved NDJSON parsing logic in network tests for more robust response detection
  • Chores

    • Minor launch and server timeout handling refinements

scroll: Remove Promise+requestAnimationFrame wrappers that caused
evaluateJSAsync to return the Promise grip instead of resolved viewport
data. Read viewport position synchronously after scroll calls.

reload: Add daemon-aware branching using start_daemon_stream /
stop_daemon_stream_draining (same pattern as navigate --with-network).
The daemon intercepts watcher events, so the direct watch_resources
approach never delivered events to the reload client.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7bc6499b-9497-4673-bbb0-b09237f49c90

📥 Commits

Reviewing files that changed from the base of the PR and between fb42d79 and 5fa7b13.

📒 Files selected for processing (3)
  • crates/ff-rdp-cli/src/commands/launch.rs
  • crates/ff-rdp-cli/src/daemon/server.rs
  • crates/ff-rdp-cli/tests/e2e/network.rs
✅ Files skipped from review due to trivial changes (3)
  • crates/ff-rdp-cli/src/daemon/server.rs
  • crates/ff-rdp-cli/tests/e2e/network.rs
  • crates/ff-rdp-cli/src/commands/launch.rs

📝 Walkthrough
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the two primary bug fixes: scroll viewport resolution and reload --wait-idle daemon mode support. It accurately reflects the main objectives of the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch iter-49/scroll-reload-fixes

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- launch.rs: Result::map().unwrap_or() → map_or()
- server.rs: Duration::from_millis(30_000) → Duration::from_secs(30)
- e2e/network.rs: Result::map_or(false, ...) → is_ok_and()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ractive ractive merged commit 93317c4 into main Apr 23, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant