Uh oh!
There was an error while loading. Please reload this page.
fix: autodetect docker host for MCP subprocesses - #8
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR implements precedence-based Docker host selection: use explicit DOCKER_HOST, else DOCKER_GIT_PROJECT_DOCKER_HOST, else if the Unix socket is missing probe tcp://host.docker.internal:2375 and use it when reachable; adds imports, applies the override to docker_command, and adds unit tests and a changelog entry. ChangesDocker Host Autodetection
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/browser.rs (1)
187-194: ⚡ Quick winTCP probe runs unconditionally, adding latency even when not needed.
host_docker_internal_docker_api_available()is called eagerly beforeselected_docker_host_overridechecks whether an explicitDOCKER_HOSTis set or the Unix socket exists. This adds up to 150ms of latency on every Docker command invocation, even when the result is never used.Consider lazy evaluation by passing a closure or deferring the probe inside
selected_docker_host_override.♻️ Proposed fix using lazy evaluation
-fn docker_host_override() -> Option<String> {- selected_docker_host_override(- nonempty_env("DOCKER_HOST").as_deref(),- nonempty_env("DOCKER_GIT_PROJECT_DOCKER_HOST").as_deref(),- Path::new("/var/run/docker.sock").exists(),- host_docker_internal_docker_api_available(),- )-}+fn docker_host_override() -> Option<String> {+ if nonempty_env("DOCKER_HOST").is_some() {+ return None;+ }++ if let Some(host) = nonempty_env("DOCKER_GIT_PROJECT_DOCKER_HOST") {+ return Some(host);+ }++ if Path::new("/var/run/docker.sock").exists() {+ return None;+ }++ if host_docker_internal_docker_api_available() {+ return Some(HOST_DOCKER_INTERNAL_DOCKER_HOST.to_string());+ }++ None+}This inlines the logic but short-circuits before the expensive TCP probe when possible. Alternatively, keep
selected_docker_host_overridefor testability and pass a closure:fndocker_host_override() -> Option<String>{selected_docker_host_override(nonempty_env("DOCKER_HOST").as_deref(),nonempty_env("DOCKER_GIT_PROJECT_DOCKER_HOST").as_deref(),Path::new("/var/run/docker.sock").exists(), || host_docker_internal_docker_api_available(),)}Then update
selected_docker_host_overrideto acceptimpl FnOnce() -> boolfor the last parameter and only invoke it when needed.🤖 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 187 - 194, The TCP probe host_docker_internal_docker_api_available() is being called eagerly in docker_host_override, adding latency; change docker_host_override to pass a zero-arg closure (e.g. || host_docker_internal_docker_api_available()) instead of its boolean result and update the signature of selected_docker_host_override to accept impl FnOnce() -> bool (or a boxed FnOnce) and only invoke that closure when the function logic actually needs the TCP probe; also update any callers and tests of selected_docker_host_override to pass a closure or adapt to the new signature so the probe stays lazy.
🤖 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 `@src/browser.rs`:
- Around line 187-194: The TCP probe host_docker_internal_docker_api_available()
is being called eagerly in docker_host_override, adding latency; change
docker_host_override to pass a zero-arg closure (e.g. ||
host_docker_internal_docker_api_available()) instead of its boolean result and
update the signature of selected_docker_host_override to accept impl FnOnce() ->
bool (or a boxed FnOnce) and only invoke that closure when the function logic
actually needs the TCP probe; also update any callers and tests of
selected_docker_host_override to pass a closure or adapt to the new signature so
the probe stays lazy.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Test Plan