feat(core): implement Rust browser connection module#2
Merged
Conversation
- Replace template sum scaffolding with typed browser container orchestration - Preserve invariant: repeated start(project) => exactly one dg-<project>-browser container - Add CLI status/start coverage and noVNC/CDP endpoint modeling
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplace Bollard/Tokio with a synchronous Docker CLI implementation, add a pure BrowserSpec model and fixed localhost noVNC/CDP URL renderers, convert the CLI main to sync, update examples/README/changelog, and rewrite tests to use pure helpers. ChangesDocker CLI Shell and Specification Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Documents the Rust-only noVNC/CDP browser connection module for issue #347
Member
Author
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/browser.rs`:
- Around line 267-296: The readiness probe in wait_for_cdp currently treats
failure to spawn the external "curl" command the same as connection failures;
modify wait_for_cdp to detect if "curl" cannot be spawned and return a clear
error immediately (e.g., call Command::new("curl").arg("--version").status()
once at start or inspect the Err from Command::new(...).status() and if it is
io::ErrorKind::NotFound return Err with a message that curl is missing), or
alternatively replace the curl loop with a Rust HTTP client (e.g., blocking
reqwest or ureq) to perform the probe; update references to cdp_probe_candidates
and the inner Command::new("curl") logic accordingly so the error distinguishes
"curl not installed" from network/connection timeouts.
In `@src/lib.rs`:
- Around line 187-198: The test
default_spec_uses_normalized_project_container_namespace is flaky because
browser_spec_from_env reads DOCKER_GIT_* env vars; fix by isolating environment:
either modify the test to temporarily clear/unset the DOCKER_GIT_PROJECT,
DOCKER_GIT_IMAGE, DOCKER_GIT_MAIN_CONTAINER (or whichever DOCKER_GIT_* keys
browser_spec_from_env reads) before calling browser_spec_from_env and restore
them after, or add a new helper browser_spec_from_values that accepts explicit
values (and update the test to call
browser_spec_from_values("docker-git-issue-347", None) instead of
browser_spec_from_env) so the test no longer depends on process environment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ea37e7-d378-4ddf-8c86-1758febfef82
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdexamples/basic_usage.rssrc/browser.rssrc/lib.rssrc/main.rstests/browser_invariants.rstests/cli_status.rstests/integration/mod.rstests/integration/sum.rstests/unit/ci-cd/changelog_parsing.rstests/unit/ci-cd/mod.rstests/unit/ci-cd/workflow_release.rstests/unit/ci-cd/workspace_manifest_resolution.rstests/unit/mod.rstests/unit/sum.rs
💤 Files with no reviewable changes (9)
- tests/integration/mod.rs
- tests/unit/ci-cd/mod.rs
- tests/integration/sum.rs
- tests/unit/ci-cd/workspace_manifest_resolution.rs
- tests/unit/mod.rs
- Cargo.toml
- tests/unit/ci-cd/workflow_release.rs
- tests/unit/ci-cd/changelog_parsing.rs
- tests/unit/sum.rs
Member
Author
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
11-11: ⚡ Quick winPin the install target for reproducible setup.
Line 11 installs from the moving default branch; docs should pin a tag or commit (
--tag/--rev) so users get deterministic behavior across time.Suggested doc tweak
-cargo install --git https://github.com/ProverCoderAI/rust-browser-connection +cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --tag vX.Y.Z +# or: +# cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --rev <commit_sha>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 11, Update the README install command so it pins a specific tag or commit instead of installing from the moving default branch; replace the current line containing "cargo install --git https://github.com/ProverCoderAI/rust-browser-connection" with a variant that includes either --tag <tag-name> or --rev <commit-hash> (e.g., use a released tag or commit SHA) to ensure deterministic installs and document which tag/commit you chose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@README.md`:
- Line 11: Update the README install command so it pins a specific tag or commit
instead of installing from the moving default branch; replace the current line
containing "cargo install --git
https://github.com/ProverCoderAI/rust-browser-connection" with a variant that
includes either --tag <tag-name> or --rev <commit-hash> (e.g., use a released
tag or commit SHA) to ensure deterministic installs and document which
tag/commit you chose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83158810-e6fb-4da7-8be8-829ad42e0bab
📒 Files selected for processing (2)
.gitignoreREADME.md
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Member
Author
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/browser.rs (3)
89-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for CDP even when reusing a running container.
The early return on the
runningpath skipswait_for_cdp(). A container can be running while Chromium/CDP is still starting or has died, sostart_browser()can return success with dead URLs.Suggested fix
let state = inspect_container_state(&spec.container_name)?; match state.as_deref() { - Some("running") => return Ok(spec.container_name.clone()), + Some("running") => { + wait_for_cdp(spec)?; + return Ok(spec.container_name.clone()); + } Some(_) => { docker(["rm", "-f", &spec.container_name], "docker rm browser")?; } None => {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/browser.rs` around lines 89 - 101, The code currently returns early when inspect_container_state(&spec.container_name) yields Some("running"), which skips calling wait_for_cdp(); update the logic in the start_browser flow so that a running container does not cause an early return — instead, proceed to call wait_for_cdp(&spec) after confirming the container is running (i.e., remove the early return in the Some("running") branch and fall through to the existing wait_for_cdp(spec)? call), while keeping the existing removal logic for non-running states (inspect_container_state, docker rm), and leaving run_browser_container and ensure_volume calls unchanged for the non-running path.
178-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExisting images never pick up embedded Dockerfile/script changes.
image_exists()only checks whether the tag already exists, and the default tag is stable for a given project. After the first build, newer binaries will keep reusing the old image, so fixes inBROWSER_DOCKERFILEandBROWSER_START_SCRIPTnever roll out unless the user manually deletes the image/container.Persist a build fingerprint or version label and rebuild/recreate when it changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/browser.rs` around lines 178 - 195, ensure_browser_image currently skips building when image_exists(&spec.image_name) is true which prevents updates to embedded BROWSER_DOCKERFILE / BROWSER_START_SCRIPT from being rolled out; compute a deterministic fingerprint (e.g., SHA256) from the build inputs (the embedded Dockerfile text, start script, and any other BrowserBuildContext inputs) inside BrowserBuildContext or ensure_browser_image, check the existing image for a matching label/fingerprint (use docker inspect or image metadata) and if the fingerprint differs, force a rebuild/recreate; when invoking docker(...) add a --label (or tag) with the fingerprint so future runs can compare and only reuse the image when the label matches (update functions: ensure_browser_image, BrowserBuildContext, and image_exists/docker invocation to support label checking).
202-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake BrowserInfo URLs respect the selected Docker
network_mode
run_browser_containerstarts with--network &spec.network_mode, butBrowserConnection::start_browseralways returnsnovnc_url=http://127.0.0.1:6080/...andcdp_url=http://127.0.0.1:9223(fromrender_novnc_url/render_cdp_url), ignoringnetwork_mode.- Since
docker rundoes not publish ports (-p/--publish),127.0.0.1:*is only reachable in the intended shared-netns case (defaultnetwork_mode = container:<project container>); named networks/other modes can leave the advertised URLs unreachable.- Either fail fast for unsupported
network_modevalues or compute/advertise reachable endpoints for the chosen mode (e.g., use the container IPs you already probe incdp_probe_candidates, or publish ports).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/browser.rs` around lines 202 - 224, BrowserInfo URLs are always rendered as localhost even when run_browser_container sets a different Docker network_mode, which makes novnc_url/cdp_url from BrowserConnection::start_browser (via render_novnc_url/render_cdp_url) unreachable for non-shared-netns modes; update the code to either fail early for unsupported network_mode values or compute reachable endpoints for the chosen mode: in BrowserConnection::start_browser detect the effective network_mode from BrowserSpec, and if it's not container:<main> or bridge with published ports then either return an explicit error or build URLs using the container IPs you already probe in cdp_probe_candidates (or by adding port publishing in run_browser_container); reference run_browser_container, BrowserConnection::start_browser, render_novnc_url, render_cdp_url, and cdp_probe_candidates when making the change so the advertised novnc_url/cdp_url reflect the actual reachable address for the configured network_mode.src/lib.rs (1)
168-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the noVNC side of the invariant exact.
is_single_browser_session()currently returnstruefor any noVNC URL that merely contains/vnc.html, even if it points at a different host or port. That can make the status check report success for mismatched endpoints.Suggested fix
pub fn is_single_browser_session(cdp_url: &str, novnc_url: &str) -> bool { - novnc_url.contains("/vnc.html") && cdp_url == render_cdp_url() + cdp_url == render_cdp_url() && novnc_url == render_novnc_url() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 168 - 170, The function is_single_browser_session currently treats any novnc_url containing "/vnc.html" as a match; change it to require the noVNC URL to exactly target the renderer host/port and the exact path "/vnc.html". Parse novnc_url and the renderer endpoint (derived from render_cdp_url()) and compare host (and port if present) plus path == "/vnc.html" rather than substring matching; keep the existing equality check for cdp_url == render_cdp_url(). Update is_single_browser_session to perform these exact comparisons (use the project's URL parsing utilities or the url crate) so only fully matching endpoints return true.
♻️ Duplicate comments (1)
src/lib.rs (1)
130-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSlugify project IDs before deriving Docker container/volume/network names
normalize_project_container_name()only trims and prefixes withdg-, so inputs likefeat/issue-347produce names likedg-feat/issue-347. That value is then used to derivemain_container_name,container_name({main}-browser),volume_name({container}-data), and defaultnetwork_mode(container:{main}), and these are passed directly to Docker CLI (docker run --name/--network,docker volume create, and the-v {volume_name}:/datamount). Docker container names and local volume names reject/(and spaces), sostart_browser()can fail for common project identifiers when theDOCKER_GIT_*overrides are unset.Suggested fix
pub fn normalize_project_container_name(project_id: &str) -> String { - let project = project_id.trim(); - let project = if project.is_empty() { - "default" - } else { - project - }; - if project.starts_with(DOCKER_GIT_CONTAINER_PREFIX) { - project.to_string() - } else { - format!("{DOCKER_GIT_CONTAINER_PREFIX}{project}") - } + let project = project_id.trim(); + let project = if project.is_empty() { "default" } else { project }; + let normalized = project + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') { + c.to_ascii_lowercase() + } else { + '-' + } + }) + .collect::<String>() + .trim_matches('-') + .to_string(); + let normalized = if normalized.is_empty() { + "default".to_string() + } else { + normalized + }; + + if normalized.starts_with(DOCKER_GIT_CONTAINER_PREFIX) { + normalized + } else { + format!("{DOCKER_GIT_CONTAINER_PREFIX}{normalized}") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 130 - 141, normalize_project_container_name currently only trims and prefixes with DOCKER_GIT_CONTAINER_PREFIX, allowing characters like '/' and spaces that are invalid for Docker names; update normalize_project_container_name to "slugify" the project_id before prefixing: trim, lowercase, replace any run of characters not matching [a-z0-9._-] (or at least non-alphanumeric) with a single '-' and trim leading/trailing '-' to produce a safe token, fall back to "default" if empty, then apply DOCKER_GIT_CONTAINER_PREFIX; ensure callers using main_container_name, container_name, volume_name and start_browser now get Docker-safe names so docker run, docker volume create and -v mounts won't fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/browser.rs`:
- Around line 89-101: The code currently returns early when
inspect_container_state(&spec.container_name) yields Some("running"), which
skips calling wait_for_cdp(); update the logic in the start_browser flow so that
a running container does not cause an early return — instead, proceed to call
wait_for_cdp(&spec) after confirming the container is running (i.e., remove the
early return in the Some("running") branch and fall through to the existing
wait_for_cdp(spec)? call), while keeping the existing removal logic for
non-running states (inspect_container_state, docker rm), and leaving
run_browser_container and ensure_volume calls unchanged for the non-running
path.
- Around line 178-195: ensure_browser_image currently skips building when
image_exists(&spec.image_name) is true which prevents updates to embedded
BROWSER_DOCKERFILE / BROWSER_START_SCRIPT from being rolled out; compute a
deterministic fingerprint (e.g., SHA256) from the build inputs (the embedded
Dockerfile text, start script, and any other BrowserBuildContext inputs) inside
BrowserBuildContext or ensure_browser_image, check the existing image for a
matching label/fingerprint (use docker inspect or image metadata) and if the
fingerprint differs, force a rebuild/recreate; when invoking docker(...) add a
--label (or tag) with the fingerprint so future runs can compare and only reuse
the image when the label matches (update functions: ensure_browser_image,
BrowserBuildContext, and image_exists/docker invocation to support label
checking).
- Around line 202-224: BrowserInfo URLs are always rendered as localhost even
when run_browser_container sets a different Docker network_mode, which makes
novnc_url/cdp_url from BrowserConnection::start_browser (via
render_novnc_url/render_cdp_url) unreachable for non-shared-netns modes; update
the code to either fail early for unsupported network_mode values or compute
reachable endpoints for the chosen mode: in BrowserConnection::start_browser
detect the effective network_mode from BrowserSpec, and if it's not
container:<main> or bridge with published ports then either return an explicit
error or build URLs using the container IPs you already probe in
cdp_probe_candidates (or by adding port publishing in run_browser_container);
reference run_browser_container, BrowserConnection::start_browser,
render_novnc_url, render_cdp_url, and cdp_probe_candidates when making the
change so the advertised novnc_url/cdp_url reflect the actual reachable address
for the configured network_mode.
In `@src/lib.rs`:
- Around line 168-170: The function is_single_browser_session currently treats
any novnc_url containing "/vnc.html" as a match; change it to require the noVNC
URL to exactly target the renderer host/port and the exact path "/vnc.html".
Parse novnc_url and the renderer endpoint (derived from render_cdp_url()) and
compare host (and port if present) plus path == "/vnc.html" rather than
substring matching; keep the existing equality check for cdp_url ==
render_cdp_url(). Update is_single_browser_session to perform these exact
comparisons (use the project's URL parsing utilities or the url crate) so only
fully matching endpoints return true.
---
Duplicate comments:
In `@src/lib.rs`:
- Around line 130-141: normalize_project_container_name currently only trims and
prefixes with DOCKER_GIT_CONTAINER_PREFIX, allowing characters like '/' and
spaces that are invalid for Docker names; update
normalize_project_container_name to "slugify" the project_id before prefixing:
trim, lowercase, replace any run of characters not matching [a-z0-9._-] (or at
least non-alphanumeric) with a single '-' and trim leading/trailing '-' to
produce a safe token, fall back to "default" if empty, then apply
DOCKER_GIT_CONTAINER_PREFIX; ensure callers using main_container_name,
container_name, volume_name and start_browser now get Docker-safe names so
docker run, docker volume create and -v mounts won't fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9346f7c7-89a2-444b-80c4-e2556c55bfcf
📒 Files selected for processing (3)
src/browser.rssrc/lib.rstests/browser_invariants.rs
skulidropek
dismissed
coderabbitai[bot]’s stale review
Resolved CodeRabbit actionable comments in a27a0bb: curl availability check + env-independent default specs.
This was referenced May 24, 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
1 participant
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
docker-git-browser-connectionmodule for issue Вынести noVNC + MCP Playright в единый модуль. docker-git#347.Short guide
Use Hermes normally; browser actions should go through
mcp_playwright_browser_*, and the same tab is visible in noVNC.Verification
source ~/.cargo/env && cargo fmt --checksource ~/.cargo/env && cargo check --lockedsource ~/.cargo/env && cargo test --lockedsource ~/.cargo/env && cargo clippy --locked -- -D warningsRuntime proof
dg-docker-git-issue-347-browserhttp://127.0.0.1:9223http://127.0.0.1:6080/vnc.html?autoconnect=true&resize=remote&path=websockify