Fix local HTTP server UI check #556
Conversation
Reviewer's GuideIntroduce a fast TCP reachability check for the local HTTP server and update UI logic to use this lightweight probe instead of the heavier process/port inspection, while keeping existing server/session semantics intact. Sequence diagram for UI polling of local HTTP server reachabilitysequenceDiagram
actor UnityEditor
participant McpConnectionSection
participant MCPServiceLocator
participant IServerManagementService
participant ServerManagementService
participant HttpEndpointUtility
participant TcpClient
UnityEditor->>McpConnectionSection: UpdateStartHttpButtonState()
McpConnectionSection->>McpConnectionSection: check httpLocalSelected
McpConnectionSection->>McpConnectionSection: check poll interval and httpServerToggleInProgress
alt need_new_poll
McpConnectionSection->>MCPServiceLocator: Server
MCPServiceLocator-->>McpConnectionSection: IServerManagementService
McpConnectionSection->>IServerManagementService: IsLocalHttpServerReachable()
IServerManagementService-->>ServerManagementService: dispatch
ServerManagementService->>HttpEndpointUtility: GetBaseUrl()
HttpEndpointUtility-->>ServerManagementService: httpUrl
ServerManagementService->>ServerManagementService: IsLocalUrl(httpUrl)
alt is_local_url
ServerManagementService->>ServerManagementService: parse Uri and port
ServerManagementService->>TcpClient: TryConnectToLocalPort(host, port, timeoutMs)
TcpClient-->>ServerManagementService: reachable bool
ServerManagementService-->>McpConnectionSection: reachable bool
McpConnectionSection->>McpConnectionSection: lastLocalServerRunning = result
else not_local_or_invalid
ServerManagementService-->>McpConnectionSection: false
McpConnectionSection->>McpConnectionSection: lastLocalServerRunning = false
end
end
McpConnectionSection->>McpConnectionSection: localServerRunning = lastLocalServerRunning
McpConnectionSection->>UnityEditor: update button enabled state and label
Updated class diagram for server management reachability checkclassDiagram
class IServerManagementService {
<<interface>>
bool IsLocalHttpServerRunning()
bool IsLocalHttpServerReachable()
}
class ServerManagementService {
bool IsLocalHttpServerRunning()
bool IsLocalHttpServerReachable()
static bool TryConnectToLocalPort(string host, int port, int timeoutMs)
bool IsLocalUrl(string url)
}
class MCPServiceLocator {
static IServerManagementService Server
}
class HttpEndpointUtility {
static string GetBaseUrl()
}
class McpConnectionSection {
double lastLocalServerRunningPollTime
bool lastLocalServerRunning
void UpdateStartHttpButtonState()
void OnHttpServerToggleClicked()
Task TryAutoStartSessionAsync()
bool IsHttpLocalSelected()
}
IServerManagementService <|.. ServerManagementService
MCPServiceLocator o-- IServerManagementService : Server
McpConnectionSection ..> MCPServiceLocator : uses
McpConnectionSection ..> IServerManagementService : calls
ServerManagementService ..> HttpEndpointUtility : uses
ServerManagementService ..> System_Net_Sockets_TcpClient : uses
class System_Net_Sockets_TcpClient {
bool Connected
Task ConnectAsync(string host, int port)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 Walkthrough🚥 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)
✏️ Tip: You can disable this entire section by setting 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TryConnectToLocalPort, you’re blocking onclient.ConnectAsyncwithWait(timeoutMs), which can be inefficient and risks thread-pool blocking; consider using a purely synchronous connect with timeout or an async pattern (e.g.,Task.WhenAnyandawait) to avoid blocking in polling paths. - The new
IsLocalHttpServerReachablere-derives the base URL and performs its own local/port checks; consider refactoring to share URL/host/port resolution logic with the existing server checks to avoid divergence in behavior over time. - In
OnHttpServerToggleClicked, the newserverRunningassignment line appears misindented relative to the surrounding code block; align its indentation to match the other statements in the method for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TryConnectToLocalPort`, you’re blocking on `client.ConnectAsync` with `Wait(timeoutMs)`, which can be inefficient and risks thread-pool blocking; consider using a purely synchronous connect with timeout or an async pattern (e.g., `Task.WhenAny` and `await`) to avoid blocking in polling paths.
- The new `IsLocalHttpServerReachable` re-derives the base URL and performs its own local/port checks; consider refactoring to share URL/host/port resolution logic with the existing server checks to avoid divergence in behavior over time.
- In `OnHttpServerToggleClicked`, the new `serverRunning` assignment line appears misindented relative to the surrounding code block; align its indentation to match the other statements in the method for consistency.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/ServerManagementService.cs:652-653` </location>
<code_context>
+ {
+ using (var client = new TcpClient())
+ {
+ var connectTask = client.ConnectAsync(target, port);
+ if (connectTask.Wait(timeoutMs) && client.Connected)
+ {
+ return true;
</code_context>
<issue_to_address>
**issue (performance):** Blocking on `ConnectAsync` with `Wait(timeoutMs)` may cause editor UI hitches.
Because this runs on the editor UI update path, `connectTask.Wait(timeoutMs)` will block the UI thread, and slow DNS or socket delays can still cause visible stutters even with a 50ms timeout. Please either move the probe to a background task and cache the result for the UI, or use an async pattern (e.g. `Task.WhenAny` with cancellation) to enforce a strict timeout without blocking the main thread.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var connectTask = client.ConnectAsync(target, port); | ||
| if (connectTask.Wait(timeoutMs) && client.Connected) |
There was a problem hiding this comment.
issue (performance): Blocking on ConnectAsync with Wait(timeoutMs) may cause editor UI hitches.
Because this runs on the editor UI update path, connectTask.Wait(timeoutMs) will block the UI thread, and slow DNS or socket delays can still cause visible stutters even with a 50ms timeout. Please either move the probe to a background task and cache the result for the UI, or use an async pattern (e.g. Task.WhenAny with cancellation) to enforce a strict timeout without blocking the main thread.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
2-2: Remove unused importSystem.Net.Sockets.This namespace is no longer used in this file after the refactoring — TCP operations are encapsulated in
ServerManagementService.TryConnectToLocalPort().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.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/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/ServerManagementService.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IServerManagementService.cs (1)
IsLocalHttpServerReachable(42-42)MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerReachable(604-625)
MCPForUnity/Editor/Services/IServerManagementService.cs (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerReachable(604-625)
MCPForUnity/Editor/Services/ServerManagementService.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 (6)
MCPForUnity/Editor/Services/IServerManagementService.cs (1)
38-42: LGTM!The new
IsLocalHttpServerReachable()method is a clean additive change to the interface. The XML documentation clearly distinguishes this fast TCP-based probe from the existingIsLocalHttpServerRunning()process inspection method, which aligns well with the PR's goal of separating UI polling concerns from session/orphan handling logic.MCPForUnity/Editor/Services/ServerManagementService.cs (2)
604-625: LGTM!The
IsLocalHttpServerReachable()implementation is well-structured with proper validation and exception handling. The 50ms timeout is appropriately aggressive for UI polling purposes, and returningfalseon any exception is a safe default.
627-671: Sound implementation with appropriate exception handling.The TCP probe logic correctly handles common localhost variations. A few notes:
- The
Task.Wait(timeoutMs)blocking call is acceptable here given the short 50ms timeout and UI-polling use case.- Exception swallowing per-host is appropriate for a probe that only needs a boolean result.
- The
TcpClientdisposal viausingproperly cleans up even when the connection attempt times out.One minor observation: connecting to
0.0.0.0as a destination typically fails (it's a bind-any address, not routable), but the fallback to127.0.0.1handles this case correctly.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
485-494: LGTM!The switch from
IsLocalHttpServerRunning()toIsLocalHttpServerReachable()in the UI polling path is the core optimization of this PR. Combined with the existing 0.75-second throttle, this should significantly reduce CPU overhead during periodic UI updates while maintaining accurate server state detection.
534-534: LGTM!Using the fast reachability check here is appropriate — the toggle decision only needs to know whether something is accepting connections on the port, not the full process identity.
594-594: LGTM!Using
IsLocalHttpServerReachable()in the startup polling loop is semantically correct — it verifies the server is actually accepting TCP connections rather than just that a process exists, which better reflects "ready to use" status.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Summary
Testing
Summary by Sourcery
Use a lightweight TCP reachability probe for local HTTP server status in the Unity editor and wire it into connection UI logic.
Enhancements:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.