Http improvements #108
Merged
Http improvements #108
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
…stering When registering with Claude Code, if a UnityMCP server already exists, remove it first before adding the new registration. This ensures the transport mode (HTTP vs stdio) is always updated to match the current UseHttpTransport EditorPref setting. Previously, if a stdio registration existed and the user tried to register with HTTP, the command would fail with 'already exists' and the old stdio configuration would remain unchanged.
…ctly
The validation code was incorrectly parsing the output of 'claude mcp get UnityMCP' by looking for JSON format ("transport": "http"), but the CLI actually returns human-readable text format ("Type: http"). This caused the transport mismatch detection to never trigger, allowing stdio to be selected in the UI while HTTP was registered with Claude Code.
Changes:
- Fix parsing logic to check for "Type: http" or "Type: stdio" in CLI output
- Add OnTransportChanged event to refresh client status when transport changes
- Wire up event handler to trigger client status refresh on transport dropdown change
This ensures that when the transport mode in Unity doesn't match what's registered with Claude Code, the UI will correctly show an error status with instructions to re-register.
…laude CLI status checks and HTTP simplifications
This commit resolves three issues with Claude Code registration: 1. UI blocking: Removed synchronous CheckStatus() call after registration that was blocking the editor. Status is now set immediately with async verification happening in the background. 2. Thread safety: Fixed "can only be called from the main thread" errors by capturing Application.dataPath and EditorPrefs.GetBool() on the main thread before spawning async status check tasks. 3. Transport mismatch detection: Transport mode changes now trigger immediate status checks to detect HTTP/stdio mismatches, instead of waiting for the 45-second refresh interval. The registration button now turns green immediately after successful registration without blocking, and properly detects transport mismatches when switching between HTTP and stdio modes.
Address code review feedback by making CheckStatusWithProjectDir thread-safe by design rather than by convention: 1. Made projectDir and useHttpTransport parameters non-nullable to prevent accidental background thread calls without captured values 2. Removed nullable fallback to EditorPrefs.GetBool() which would cause thread safety violations if called from background threads 3. Added ArgumentNullException for null projectDir instead of falling back to Application.dataPath (which is main-thread only) 4. Added XML documentation clearly stating threading contracts: - CheckStatus() must be called from main thread - CheckStatusWithProjectDir() is safe for background threads 5. Removed unreachable else branch in async status check code These changes make it impossible to misuse the API from background threads, with compile-time enforcement instead of runtime errors.
…-tasks Fix local HTTP server lifecycle and TCP connect observation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR introduces comprehensive HTTP server lifecycle management for the Claude MCP integration, adding transport-aware status checking, deterministic server startup/shutdown with PID-based handshakes, fast-fail command logic on the server side, and consolidated UI for local vs. remote HTTP transport modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant SrvMgmt as ServerManagementService
participant Proc as Process<br/>(Terminal)
participant PidFile as PID File
participant TCP as TCP Port
Editor->>SrvMgmt: StartLocalHttpServer()
activate SrvMgmt
SrvMgmt->>SrvMgmt: StopAnyExistingServer()
SrvMgmt->>TCP: CheckPortAvailable()
TCP-->>SrvMgmt: ✓ Port free
SrvMgmt->>SrvMgmt: GeneratePerLaunchToken()
SrvMgmt->>SrvMgmt: BuildServerCommand(token,port)
SrvMgmt->>Proc: LaunchTerminal(command)
activate Proc
SrvMgmt->>PidFile: WriteHandshake(PID,port,token)
SrvMgmt-->>Editor: Server started
deactivate SrvMgmt
Editor->>SrvMgmt: IsLocalHttpServerRunning()
activate SrvMgmt
SrvMgmt->>PidFile: ReadHandshake()
SrvMgmt->>SrvMgmt: ValidateToken()
SrvMgmt->>TCP: ProbeForAcceptingConnections(retries)
alt TCP responds
TCP-->>SrvMgmt: Connection OK
SrvMgmt-->>Editor: true
else TCP timeout
SrvMgmt-->>Editor: false
end
deactivate SrvMgmt
Editor->>SrvMgmt: StopManagedLocalHttpServer()
activate SrvMgmt
SrvMgmt->>PidFile: ReadHandshake(validateToken)
SrvMgmt->>Proc: TerminateProcess(PID)
activate Proc
Proc-->>SrvMgmt: ✓ Terminated
deactivate Proc
SrvMgmt->>PidFile: ClearHandshake()
SrvMgmt-->>Editor: Stopped
deactivate SrvMgmt
sequenceDiagram
participant Client as Claude<br/>Client
participant Server as MCP<br/>Server
participant Hub as PluginHub
participant Unity as Unity<br/>Plugin
Client->>Server: SendCommand(read_console)
activate Server
Server->>Hub: send_command_for_instance(session_id, read_console)
activate Hub
alt Fast-path command (read_console, ping, etc.)
Hub->>Hub: Use fast timeout (3s default)
Hub->>Hub: ReadinessProbe() for fast-path
alt Not ready
Hub-->>Server: MCPResponse(retry_hint)
else Ready
Hub->>Unity: Send command
end
else Regular command
Hub->>Hub: Use standard timeout
Hub->>Unity: Send command
end
activate Unity
alt Normal completion
Unity-->>Hub: Response
deactivate Unity
Hub->>Hub: _handle_command_result()
Hub-->>Server: Success response
else Timeout
Hub->>Hub: OnTimeout (fast-path?)
alt Fast-path timeout
Hub-->>Server: MCPResponse(retry_hint)
else Regular timeout
Hub-->>Server: Timeout error
end
else Plugin disconnects
Hub->>Hub: on_disconnect(session_id)
Hub->>Hub: PluginDisconnectedError
Hub-->>Server: MCPResponse(retry_hint)
end
deactivate Hub
Server-->>Client: Response/Retry hint
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (21)
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 |
Greptile Summary
Important Files Changed
Confidence score: 3/5
Sequence DiagramsequenceDiagram
participant User
participant McpConnectionSection
participant ServerManagementService
participant BridgeControlService
participant TransportManager
participant WebSocketTransportClient
participant PluginHub
User->>McpConnectionSection: "Click Start Server button"
McpConnectionSection->>ServerManagementService: "StartLocalHttpServer()"
ServerManagementService->>ServerManagementService: "StopLocalHttpServerInternal(quiet: true)"
ServerManagementService->>ServerManagementService: "CreateTerminalProcessStartInfo(command)"
ServerManagementService->>ServerManagementService: "Process.Start(startInfo)"
ServerManagementService->>ServerManagementService: "StoreLocalHttpServerHandshake(pidFile, token)"
ServerManagementService-->>McpConnectionSection: "true"
McpConnectionSection->>McpConnectionSection: "TryAutoStartSessionAfterHttpServerAsync()"
McpConnectionSection->>McpConnectionSection: "WaitForHttpServerAcceptingConnectionsAsync()"
McpConnectionSection->>BridgeControlService: "StartAsync()"
BridgeControlService->>TransportManager: "StartAsync(TransportMode.Http)"
TransportManager->>WebSocketTransportClient: "StartAsync()"
WebSocketTransportClient->>PluginHub: "ConnectAsync(endpointUri)"
PluginHub-->>WebSocketTransportClient: "WelcomeMessage"
WebSocketTransportClient->>PluginHub: "RegisterMessage"
PluginHub-->>WebSocketTransportClient: "RegisteredMessage(session_id)"
WebSocketTransportClient->>PluginHub: "RegisterToolsMessage"
WebSocketTransportClient-->>TransportManager: "true"
TransportManager-->>BridgeControlService: "true"
BridgeControlService-->>McpConnectionSection: "true"
McpConnectionSection->>BridgeControlService: "VerifyAsync()"
BridgeControlService-->>McpConnectionSection: "BridgeVerificationResult"
McpConnectionSection->>McpConnectionSection: "UpdateConnectionStatus()"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Legacy safety: stdio may have been started outside TransportManager state. | ||
| if (otherMode == TransportMode.Stdio) | ||
| { | ||
| try { StdioBridgeHost.Stop(); } catch { } |
There was a problem hiding this comment.
style: Empty catch block silently swallows all exceptions. Consider logging at debug level or checking for specific exception types.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Services/BridgeControlService.cs
Line: 100:100
Comment:
**style:** Empty catch block silently swallows all exceptions. Consider logging at debug level or checking for specific exception types.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+46
to
+47
| if not response.get("success", True): | ||
| return MCPResponse(**response) |
There was a problem hiding this comment.
logic: Using response.get("success", True) means responses without a 'success' field are treated as successful, which may mask actual failures. Should responses without a 'success' field be treated as successful by default?
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/resources/editor_state.py
Line: 46:47
Comment:
**logic:** Using `response.get("success", True)` means responses without a 'success' field are treated as successful, which may mask actual failures. Should responses without a 'success' field be treated as successful by default?
How can I resolve this? If you propose a fix, please make it concise.| if not response.get("success", True): | ||
| return MCPResponse(**response) | ||
| if response.get("data") is None: | ||
| return MCPResponse(success=False, error="Editor state missing 'data' payload", data=response) |
There was a problem hiding this comment.
style: Including the original response as data in the error response could expose internal implementation details or create confusing nested structures
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/resources/editor_state.py
Line: 49:49
Comment:
**style:** Including the original response as `data` in the error response could expose internal implementation details or create confusing nested structures
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+407
to
+411
| // Parse the output to determine registered transport mode | ||
| // The CLI output format contains "Type: http" or "Type: stdio" | ||
| bool registeredWithHttp = getStdout.Contains("Type: http", StringComparison.OrdinalIgnoreCase); | ||
| bool registeredWithStdio = getStdout.Contains("Type: stdio", StringComparison.OrdinalIgnoreCase); | ||
|
|
There was a problem hiding this comment.
style: Uses string.Contains() to parse CLI output format - this could be fragile if the CLI output format changes
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
Line: 407:411
Comment:
**style:** Uses string.Contains() to parse CLI output format - this could be fragile if the CLI output format changes
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+53
to
+59
| if isinstance(count, str) and count.strip().lower() in ("all", "*"): | ||
| count = None | ||
| else: | ||
| count = coerce_int(count) | ||
|
|
||
| if action == "get" and count is None: | ||
| count = 200 |
There was a problem hiding this comment.
logic: Logic issue: when count="all" is passed, it's converted to None, but then line 58-59 immediately sets it back to 200, defeating the purpose of the "all" parameter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/read_console.py
Line: 53:59
Comment:
**logic:** Logic issue: when count="all" is passed, it's converted to None, but then line 58-59 immediately sets it back to 200, defeating the purpose of the "all" parameter.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // Best-effort nudge: if we're posting from a background thread (e.g., websocket receive), | ||
| // encourage Unity to run a loop iteration so the posted callback can execute even when unfocused. | ||
| try { EditorApplication.QueuePlayerLoopUpdate(); } catch { } |
There was a problem hiding this comment.
style: Empty catch block silently suppresses all exceptions from QueuePlayerLoopUpdate
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs
Line: 155:155
Comment:
**style:** Empty catch block silently suppresses all exceptions from QueuePlayerLoopUpdate
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+696
to
+697
| var connectTask = client.ConnectAsync(normalizedHost, port); | ||
| var completed = await Task.WhenAny(connectTask, Task.Delay(250)); |
There was a problem hiding this comment.
style: TcpClient.ConnectAsync can throw synchronously if the host is invalid. Consider wrapping this in a try-catch to handle immediate failures gracefully.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Line: 696:697
Comment:
**style:** TcpClient.ConnectAsync can throw synchronously if the host is invalid. Consider wrapping this in a try-catch to handle immediate failures gracefully.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+142
to
+148
| if command_type in cls._FAST_FAIL_COMMANDS: | ||
| try: | ||
| fast_timeout = float(os.environ.get("UNITY_MCP_FAST_COMMAND_TIMEOUT", "3")) | ||
| except Exception: | ||
| fast_timeout = 3.0 | ||
| unity_timeout_s = fast_timeout | ||
| server_wait_s = fast_timeout |
There was a problem hiding this comment.
style: Environment variable UNITY_MCP_FAST_COMMAND_TIMEOUT lacks validation - should ensure it's a positive number
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/plugin_hub.py
Line: 142:148
Comment:
**style:** Environment variable `UNITY_MCP_FAST_COMMAND_TIMEOUT` lacks validation - should ensure it's a positive number
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| try: | ||
| requested_s = float(requested) | ||
| # Clamp to a sane upper bound to avoid accidental infinite hangs. | ||
| requested_s = max(1.0, min(requested_s, 60.0 * 60.0)) |
There was a problem hiding this comment.
style: Magic number 60*60 (3600 seconds) for max timeout should be a named constant
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/plugin_hub.py
Line: 164:164
Comment:
**style:** Magic number 60*60 (3600 seconds) for max timeout should be a named constant
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+452
to
+454
| max_wait_s = float(os.environ.get("UNITY_MCP_SESSION_READY_WAIT_SECONDS", "6")) | ||
| except Exception: | ||
| max_wait_s = 6.0 |
There was a problem hiding this comment.
style: Duplicate timeout environment variable handling - could be extracted to a helper method
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/plugin_hub.py
Line: 452:454
Comment:
**style:** Duplicate timeout environment variable handling - could be extracted to a helper method
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
This was referenced Jan 4, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.