Use localhost to ping server if server binds to 0.0.0.0 #542
+10
−8
Conversation
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
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideWebSocket client startup logic now normalizes server URLs that bind to 0.0.0.0 by replacing that host with localhost before constructing the WebSocket endpoint URI, ensuring client connections succeed when the server listens on all interfaces. Sequence diagram for WebSocketTransportClient StartAsync with 0_0_0_0 to localhost normalizationsequenceDiagram
actor Developer
participant WebSocketTransportClient
participant HttpEndpointUtility
participant CancellationTokenSource
Developer->>WebSocketTransportClient: StartAsync()
WebSocketTransportClient->>WebSocketTransportClient: StopAsync()
WebSocketTransportClient->>CancellationTokenSource: new CancellationTokenSource()
WebSocketTransportClient->>HttpEndpointUtility: GetBaseUrl()
HttpEndpointUtility-->>WebSocketTransportClient: baseUrl (may contain 0.0.0.0)
WebSocketTransportClient->>WebSocketTransportClient: baseUrl = baseUrl.Replace(0.0.0.0, localhost)
WebSocketTransportClient->>WebSocketTransportClient: _endpointUri = BuildWebSocketUri(baseUrl)
WebSocketTransportClient->>WebSocketTransportClient: _sessionId = null
WebSocketTransportClient->>WebSocketTransportClient: EstablishConnectionAsync(_lifecycleCts.Token)
WebSocketTransportClient-->>Developer: bool (connectionEstablished)
Class diagram for updated WebSocketTransportClient StartAsync URL handlingclassDiagram
class WebSocketTransportClient {
- CancellationTokenSource _lifecycleCts
- Uri _endpointUri
- string _sessionId
+ Task~bool~ StartAsync()
+ Task StopAsync()
- Uri BuildWebSocketUri(string baseUrl)
- Task~bool~ EstablishConnectionAsync(CancellationToken cancellationToken)
}
class HttpEndpointUtility {
+ static string GetBaseUrl()
}
WebSocketTransportClient ..> HttpEndpointUtility : uses GetBaseUrl
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughStartAsync in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The direct string replacement
baseUrl.Replace("0.0.0.0", "localhost")is a bit brittle; consider parsing the URL as aUriand only swapping theHostwhen it equals0.0.0.0to avoid accidental replacements in paths or query parameters. - It might be cleaner to move the
0.0.0.0→localhosttranslation intoHttpEndpointUtility.GetBaseUrl()orBuildWebSocketUriso that this behavior is centralized and consistently applied across any other consumers of the base URL.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The direct string replacement `baseUrl.Replace("0.0.0.0", "localhost")` is a bit brittle; consider parsing the URL as a `Uri` and only swapping the `Host` when it equals `0.0.0.0` to avoid accidental replacements in paths or query parameters.
- It might be cleaner to move the `0.0.0.0` → `localhost` translation into `HttpEndpointUtility.GetBaseUrl()` or `BuildWebSocketUri` so that this behavior is centralized and consistently applied across any other consumers of the base URL.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs:85-92` </location>
<code_context>
+
+ // Get base URL and replace 0.0.0.0 with localhost for client connections
+ // 0.0.0.0 is only valid for server binding, not client connections
+ string baseUrl = HttpEndpointUtility.GetBaseUrl();
+ baseUrl = baseUrl.Replace("0.0.0.0", "localhost");
+ _endpointUri = BuildWebSocketUri(baseUrl);
_sessionId = null;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use URI parsing instead of raw string replacement for the host adjustment.
This approach will replace any "0.0.0.0" substring, not just the host. Prefer parsing `baseUrl` as a `Uri`, checking `uri.Host == "0.0.0.0"`, and then using a `UriBuilder` with `Host = "localhost"` to ensure only the host component is changed.
```suggestion
_lifecycleCts = new CancellationTokenSource();
// Get base URL and normalize 0.0.0.0 to localhost for client connections
// 0.0.0.0 is only valid for server binding, not client connections
string baseUrl = HttpEndpointUtility.GetBaseUrl();
var baseUri = new Uri(baseUrl, UriKind.Absolute);
if (baseUri.Host == "0.0.0.0")
{
var uriBuilder = new UriBuilder(baseUri)
{
Host = "localhost"
};
baseUri = uriBuilder.Uri;
}
_endpointUri = BuildWebSocketUri(baseUri.ToString());
_sessionId = null;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs:
- Around line 86-92: The code currently uses string.Replace on the whole URL
which can corrupt other parts; instead parse the base URL returned by
HttpEndpointUtility.GetBaseUrl() into a Uri/UriBuilder, check the host and only
rewrite it if the host equals "0.0.0.0" (and optionally handle "127.0.0.1"
mapping if you prefer "localhost"), then rebuild the URL and pass that to
BuildWebSocketUri; update the block that sets _endpointUri (and leaves
_sessionId null) to use the Uri/UriBuilder host-only replacement and fall back
to the original string if parsing fails.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
HttpEndpointUtility(12-85)GetBaseUrl(20-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
86-92: No other client transports require the same normalization.WebSocketTransportClient is the only client transport using
HttpEndpointUtility.GetBaseUrl()for establishing connections. StdioTransportClient uses a different mechanism (StdioBridgeHost), and ServerManagementService's calls toGetBaseUrl()are limited to server lifecycle management, not client connections.
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
Outdated
Show resolved
Hide resolved
kripergvg
force-pushed
the
main
branch
2 times, most recently
from
420d2e3 to
5389b01
Compare
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.
Problem: When the MCP server is configured to bind to 0.0.0.0:8080 (I am doing it locally to be able to access MCP from my local windows and from WSL), the WebSocket client would attempt to connect to 0.0.0.0, which fails because 0.0.0.0 is only valid for server binding, not client connections.
Solution: Added automatic URL translation that replaces 0.0.0.0 with localhost before establishing the WebSocket connection. This allows the server to bind on all interfaces while ensuring the client connects to the correct loopback address.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.