fix: allow standalone browser MCP startup#7
Merged
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors browser container provisioning to support deterministic per-project port allocation and runtime-resolved endpoints. The core change introduces BrowserPorts for port assignment, BrowserRuntime to carry resolved CDP/noVNC URLs, network inspection with fallback logic, and endpoint probing that returns URLs instead of just success indicators. ChangesBrowser Port Allocation and Runtime Resolution
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib.rs (2)
48-59: 💤 Low valuePort collision risk with 400 possible offsets.
The hash modulo 400 means different project IDs can collide to the same ports. If two projects with colliding hashes run simultaneously on the same host, they'll conflict. The inline comment acknowledges this ("usually map to different offsets"), so this appears intentional.
If collision avoidance becomes important, consider widening the offset range or adding runtime port availability checks.
🤖 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 48 - 59, compute_browser_ports currently uses hash % 400 which risks port collisions; update compute_browser_ports (and related constants BROWSER_VNC_PORT, BROWSER_NOVNC_PORT, BROWSER_CDP_PORT) to reduce collisions by either increasing the offset space (e.g., use hash % 2000 or a configurable MAX_OFFSET) or implement a runtime availability probe: compute an initial offset from normalize_project_container_name, then test the candidate ports (vnc/novnc/cdp) for availability and, if any are in use, increment the offset (wrapping within the allowed range) until a free triplet is found or return an error; ensure the probe logic is placed in or called from compute_browser_ports and document the new MAX_OFFSET/config option.
215-218: 💤 Low valueFunction name may mislead after logic change.
is_single_browser_sessionnow only validates URL shapes (noVNC path present, CDP uses HTTP/HTTPS scheme) rather than verifying both URLs point to the same session. The name suggests session identity verification, but the implementation is a format check.Consider renaming to
are_valid_browser_urlsor similar, or documenting that "single session" is an assumption based on URL validity rather than an enforced invariant.🤖 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 215 - 218, The function is_single_browser_session no longer checks session identity and only validates URL shapes; rename the function to are_valid_browser_urls (or another clear name) and update all callers to use are_valid_browser_urls(cdp_url, novnc_url), or alternatively keep the existing name but add a doc comment on is_single_browser_session that explicitly states it only checks novnc_url contains "/vnc.html" and cdp_url starts with "http://" or "https://" (i.e., a format check, not identity verification); update any unit tests and references to match the new name or documentation to avoid misleading semantics.
🤖 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 `@README.md`:
- Line 55: The curl health-check example uses a Host header port 9222 that
doesn't match the URL port 9593; update the example curl command (the line
starting with "curl -H 'Host: 127.0.0.1:9222'
http://127.0.0.1:9593/json/version") so the Host header port is 9593 (e.g.,
"Host: 127.0.0.1:9593") to match the CDP port shown earlier.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 48-59: compute_browser_ports currently uses hash % 400 which risks
port collisions; update compute_browser_ports (and related constants
BROWSER_VNC_PORT, BROWSER_NOVNC_PORT, BROWSER_CDP_PORT) to reduce collisions by
either increasing the offset space (e.g., use hash % 2000 or a configurable
MAX_OFFSET) or implement a runtime availability probe: compute an initial offset
from normalize_project_container_name, then test the candidate ports
(vnc/novnc/cdp) for availability and, if any are in use, increment the offset
(wrapping within the allowed range) until a free triplet is found or return an
error; ensure the probe logic is placed in or called from compute_browser_ports
and document the new MAX_OFFSET/config option.
- Around line 215-218: The function is_single_browser_session no longer checks
session identity and only validates URL shapes; rename the function to
are_valid_browser_urls (or another clear name) and update all callers to use
are_valid_browser_urls(cdp_url, novnc_url), or alternatively keep the existing
name but add a doc comment on is_single_browser_session that explicitly states
it only checks novnc_url contains "/vnc.html" and cdp_url starts with "http://"
or "https://" (i.e., a format check, not identity verification); update any unit
tests and references to match the new name or documentation to avoid misleading
semantics.
🪄 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: 3c0f4306-f05c-48bf-9469-2eeb56f5c450
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlREADME.mdchangelog.d/20260524_141210_browser_connection_mcp.mdsrc/browser.rssrc/lib.rstests/browser_invariants.rstests/cli_status.rs
| open http://127.0.0.1:6080/vnc.html?autoconnect=true\&resize=remote\&path=websockify | ||
| # Use the CDP/noVNC URLs printed by `start`; in containerized Docker hosts the | ||
| # module may choose the browser container IP instead of 127.0.0.1 host ports. | ||
| curl -H 'Host: 127.0.0.1:9222' http://127.0.0.1:9593/json/version |
There was a problem hiding this comment.
Port mismatch in Host header and URL.
The curl health check uses port 9222 in the Host header but port 9593 in the URL. Based on the example output at line 47, the CDP port for this project is 9593, so the Host header should match.
📝 Proposed fix
-curl -H 'Host: 127.0.0.1:9222' http://127.0.0.1:9593/json/version
+curl -H 'Host: 127.0.0.1:9593' http://127.0.0.1:9593/json/version📝 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.
Suggested change
| curl -H 'Host: 127.0.0.1:9222' http://127.0.0.1:9593/json/version | |
| curl -H 'Host: 127.0.0.1:9593' http://127.0.0.1:9593/json/version |
🤖 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 55, The curl health-check example uses a Host header port
9222 that doesn't match the URL port 9593; update the example curl command (the
line starting with "curl -H 'Host: 127.0.0.1:9222'
http://127.0.0.1:9593/json/version") so the Host header port is 9593 (e.g.,
"Host: 127.0.0.1:9593") to match the CDP port shown earlier.
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
Verification