Fix Claude Code Registration and HTTP Transport Issues #497
+83
−29
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.
Contributor
Reviewer's GuideRefactors Claude CLI MCP status checking and registration to be non-blocking, thread-safe, and transport-aware, and wires the transport selector UI to trigger immediate, correct status refreshes when HTTP/stdio settings change. Sequence diagram for transport change triggering immediate Claude CLI status refreshsequenceDiagram
actor User
participant MCPForUnityEditorWindow
participant McpConnectionSection
participant McpClientConfigSection
participant ClaudeCliMcpConfigurator
participant BackgroundTask
User->>McpConnectionSection: Change transport protocol
McpConnectionSection-->>McpConnectionSection: UpdateHttpFieldVisibility()
McpConnectionSection-->>McpConnectionSection: RefreshHttpUi()
McpConnectionSection-->>MCPForUnityEditorWindow: OnManualConfigUpdateRequested
MCPForUnityEditorWindow->>McpClientConfigSection: UpdateManualConfiguration()
McpConnectionSection-->>MCPForUnityEditorWindow: OnTransportChanged
MCPForUnityEditorWindow->>McpClientConfigSection: RefreshSelectedClient(forceImmediate=true)
McpClientConfigSection-->>McpClientConfigSection: shouldForceImmediate = true
McpClientConfigSection->>McpClientConfigSection: RefreshClientStatus(client, forceImmediate=true)
McpClientConfigSection->>McpClientConfigSection: RefreshClaudeCliStatus(client, forceImmediate=true)
alt First time or stale status
McpClientConfigSection-->>McpClientConfigSection: needsRefresh = true
else Recently checked and fresh
McpClientConfigSection-->>McpClientConfigSection: needsRefresh may be false
end
McpClientConfigSection-->>McpClientConfigSection: Capture projectDir = Path.GetDirectoryName(Application.dataPath)
McpClientConfigSection-->>McpClientConfigSection: Capture useHttpTransport = EditorPrefs.GetBool(UseHttpTransport)
McpClientConfigSection-->>McpClientConfigSection: statusRefreshInFlight.Add(client)
McpClientConfigSection-->>McpClientConfigSection: ApplyStatusToUi(showChecking=true)
McpClientConfigSection->>BackgroundTask: Task.Run(CheckStatusWithProjectDir(projectDir, useHttpTransport))
BackgroundTask->>ClaudeCliMcpConfigurator: CheckStatusWithProjectDir(projectDir, useHttpTransport, attemptAutoRewrite=false)
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: ExecPath.TryRun(claudePath, "mcp list")
alt UnityMCP not found
ClaudeCliMcpConfigurator-->>ClaudeCliMcpConfigurator: client.SetStatus(NotConfigured)
else UnityMCP found
ClaudeCliMcpConfigurator->>ClaudeCliMcpConfigurator: ExecPath.TryRun(claudePath, "mcp get UnityMCP")
ClaudeCliMcpConfigurator-->>ClaudeCliMcpConfigurator: Parse Type http or Type stdio
alt Transport mismatch
ClaudeCliMcpConfigurator-->>ClaudeCliMcpConfigurator: client.SetStatus(Error, mismatch message)
else Transport matches
ClaudeCliMcpConfigurator-->>ClaudeCliMcpConfigurator: client.SetStatus(Configured)
end
end
ClaudeCliMcpConfigurator-->>BackgroundTask: Return McpStatus
BackgroundTask-->>McpClientConfigSection: ContinueWith(result, faulted)
McpClientConfigSection-->>McpClientConfigSection: statusRefreshInFlight.Remove(client)
McpClientConfigSection-->>McpClientConfigSection: lastStatusChecks[client] = DateTime.UtcNow
McpClientConfigSection-->>McpClientConfigSection: ApplyStatusToUi(showChecking=false)
McpClientConfigSection-->>User: Updated non-blocking status indicator
Sequence diagram for non-blocking Claude CLI registration and status handlingsequenceDiagram
actor User
participant ClaudeCliMcpConfigurator
participant ExecPath
participant ClaudeCliCLI as ClaudeCLI
participant MCPClient as McpClient
participant McpClientConfigSection
participant BackgroundTask
User->>ClaudeCliMcpConfigurator: Register()
ClaudeCliMcpConfigurator->>ExecPath: TryRun(claudePath, "mcp get UnityMCP")
alt Existing registration found
ExecPath-->>ClaudeCliMcpConfigurator: serverExists = true
ClaudeCliMcpConfigurator->>ExecPath: TryRun(claudePath, "mcp remove UnityMCP")
alt Remove fails
ExecPath-->>ClaudeCliMcpConfigurator: Failure, log warning
else Remove succeeds
ExecPath-->>ClaudeCliMcpConfigurator: Removed
end
else No existing registration
ExecPath-->>ClaudeCliMcpConfigurator: serverExists = false
end
ClaudeCliMcpConfigurator->>ExecPath: TryRun(claudePath, registerArgs)
alt Register fails
ExecPath-->>ClaudeCliMcpConfigurator: stdout, stderr
ClaudeCliMcpConfigurator-->>User: Throw InvalidOperationException
else Register succeeds
ExecPath-->>ClaudeCliMcpConfigurator: Success
ClaudeCliMcpConfigurator->>MCPClient: SetStatus(Configured)
ClaudeCliMcpConfigurator-->>User: Return without blocking status check
end
User->>McpClientConfigSection: Trigger manual or automatic refresh
McpClientConfigSection-->>McpClientConfigSection: Capture projectDir and useHttpTransport on main thread
McpClientConfigSection->>BackgroundTask: Task.Run(CheckStatusWithProjectDir(projectDir, useHttpTransport))
BackgroundTask->>ClaudeCliMcpConfigurator: CheckStatusWithProjectDir(projectDir, useHttpTransport)
ClaudeCliMcpConfigurator->>ExecPath: TryRun(claudePath, "mcp list")
alt UnityMCP present
ClaudeCliMcpConfigurator->>ExecPath: TryRun(claudePath, "mcp get UnityMCP")
ClaudeCliMcpConfigurator-->>ClaudeCliMcpConfigurator: Parse Type http or Type stdio
alt Transport mismatch
ClaudeCliMcpConfigurator->>MCPClient: SetStatus(Error, mismatch message)
else Transport matches
ClaudeCliMcpConfigurator->>MCPClient: SetStatus(Configured)
end
else UnityMCP absent
ClaudeCliMcpConfigurator->>MCPClient: SetStatus(NotConfigured)
end
ClaudeCliMcpConfigurator-->>BackgroundTask: Return McpStatus
BackgroundTask-->>McpClientConfigSection: Update status and UI asynchronously
Updated class diagram for Claude CLI configuration and transport-aware status flowclassDiagram
class McpClient {
+McpStatus status
+SetStatus(status, message)
}
class McpClientConfiguratorBase {
<<abstract>>
+McpClient client
+GetConfigPath() string
+CheckStatus(attemptAutoRewrite bool) McpStatus
+Register()
+Unregister()
+GetManualSnippet() string
}
class ClaudeCliMcpConfigurator {
+ClaudeCliMcpConfigurator(McpClient client)
+GetConfigPath() string
+CheckStatus(attemptAutoRewrite bool) McpStatus
+CheckStatusWithProjectDir(projectDir string, useHttpTransport bool?, attemptAutoRewrite bool) McpStatus
-Register()
-Unregister()
}
class IMcpClientConfigurator {
<<interface>>
+GetConfigPath() string
+CheckStatus(attemptAutoRewrite bool) McpStatus
}
class McpClientConfigSection {
-List~IMcpClientConfigurator~ configurators
-Dictionary~IMcpClientConfigurator, DateTime~ lastStatusChecks
-HashSet~IMcpClientConfigurator~ statusRefreshInFlight
-int selectedClientIndex
+RefreshSelectedClient(forceImmediate bool)
+RefreshClientStatus(client IMcpClientConfigurator, forceImmediate bool)
+RefreshClaudeCliStatus(client IMcpClientConfigurator, forceImmediate bool)
+UpdateManualConfiguration()
+UpdateClaudeCliPathVisibility()
}
class McpConnectionSection {
-enum TransportProtocol
+event Action OnManualConfigUpdateRequested
+event Action OnTransportChanged
+RegisterCallbacks()
}
class MCPForUnityEditorWindow {
-McpConnectionSection connectionSection
-McpClientConfigSection clientConfigSection
+CreateGUI()
}
class EditorPrefKeys {
<<static>>
+UseHttpTransport string
}
McpClientConfiguratorBase <|-- ClaudeCliMcpConfigurator
IMcpClientConfigurator <|.. ClaudeCliMcpConfigurator
McpClientConfigSection --> IMcpClientConfigurator : manages
McpClientConfigSection --> ClaudeCliMcpConfigurator : special handling
ClaudeCliMcpConfigurator --> McpClient : updates status
McpConnectionSection --> MCPForUnityEditorWindow : raises events
MCPForUnityEditorWindow --> McpConnectionSection : owns
MCPForUnityEditorWindow --> McpClientConfigSection : owns
McpConnectionSection --> McpClientConfigSection : OnManualConfigUpdateRequested
McpConnectionSection --> McpClientConfigSection : OnTransportChanged triggers RefreshSelectedClient(forceImmediate)
McpClientConfigSection --> EditorPrefKeys : reads UseHttpTransport
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
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. 📝 WalkthroughWalkthroughAdds transport-aware status checks and registration cleanup for the Claude CLI configurator, exposes an OnTransportChanged event from the connection UI, and triggers an immediate client-config refresh when transport changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Conn as McpConnectionSection
participant Editor as MCPForUnityEditorWindow
participant ClientUI as McpClientConfigSection
participant Config as ClaudeCliMcpConfigurator
participant CLI as MCP CLI
User->>Conn: Change transport setting
Conn->>Conn: Update internal transport mode
Conn->>Conn: Invoke OnTransportChanged
Conn->>Editor: OnTransportChanged event
rect rgb(230,245,255)
Editor->>ClientUI: RefreshSelectedClient(forceImmediate:true)
ClientUI->>ClientUI: Capture projectDir & useHttpTransport (main thread)
ClientUI->>ClientUI: Start Task.Run for status check
end
rect rgb(240,250,240)
ClientUI->>Config: CheckStatusWithProjectDir(projectDir, useHttpTransport)
Config->>CLI: Run "mcp list"
CLI-->>Config: list output
alt UnityMCP present
Config->>CLI: Run "mcp get UnityMCP"
CLI-->>Config: config (contains Type: http|stdio)
Config->>Config: Parse registered transport and compare
alt mismatch
Config->>ClientUI: Return error status + log warning
else match
Config->>ClientUI: Return Configured status
end
else Not registered
Config->>ClientUI: Return NotConfigured
end
end
ClientUI->>Editor: Display updated status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 new
CheckStatusWithProjectDirhelper still falls back toEditorPrefs.GetBoolwhenuseHttpTransportis null, which will break if any future callers invoke it from a background thread without passing the captured value; consider either enforcing non-nulluseHttpTransport(e.g., make it non-nullable) or explicitly guarding that code path to require main-thread execution. - Now that background tasks call
CheckStatusWithProjectDirdirectly, it might be worth centralizing all Claude CLI status checks (including those viaMCPServiceLocator.Client.CheckClientStatus) through this method to avoid divergent code paths and ensure consistent transport mismatch handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `CheckStatusWithProjectDir` helper still falls back to `EditorPrefs.GetBool` when `useHttpTransport` is null, which will break if any future callers invoke it from a background thread without passing the captured value; consider either enforcing non-null `useHttpTransport` (e.g., make it non-nullable) or explicitly guarding that code path to require main-thread execution.
- Now that background tasks call `CheckStatusWithProjectDir` directly, it might be worth centralizing all Claude CLI status checks (including those via `MCPServiceLocator.Client.CheckClientStatus`) through this method to avoid divergent code paths and ensure consistent transport mismatch handling.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs:343-346` </location>
<code_context>
+ }
+
+ // Internal version that accepts projectDir and useHttpTransport to allow calling from background threads
+ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool? useHttpTransport, bool attemptAutoRewrite = true)
{
try
</code_context>
<issue_to_address>
**issue (bug_risk):** Access to EditorPrefs from this method can occur on a background thread when useHttpTransport is null.
Because this overload may be called from background threads and still falls back to `EditorPrefs.GetBool` when `useHttpTransport` is null, there’s a latent threading hazard if future callers invoke it off the main thread. Please either (a) enforce that `useHttpTransport` is non-null for any off-main-thread usage (with guards/asserts), or (b) refactor so that all `EditorPrefs` access is done in a clearly main-thread-only helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
356-360: Verify thread safety:Application.dataPathis main-thread only.The fallback to
Application.dataPathwhenprojectDiris null will throw if called from a background thread. The current design expects callers to always provideprojectDirwhen calling from background threads.Since
CheckStatus()(the public entry point) calls this withnull, it must only be called from the main thread. The internalCheckStatusWithProjectDiris called with captured values fromMcpClientConfigSection.RefreshClaudeCliStatus, which correctly capturesprojectDirbeforeTask.Run. This looks correct, but the implicit contract should be documented.Consider adding a comment or assertion to make the threading contract explicit:
// Use provided projectDir or get it from Application.dataPath (main thread only) if (string.IsNullOrEmpty(projectDir)) { + // This path is only safe from the main thread projectDir = Path.GetDirectoryName(Application.dataPath); }MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
347-355: Defensive branch for non-Claude clients is unreachable but harmless.The
elsebranch (lines 352-355) will never execute becauseRefreshClaudeCliStatusis only called whenclient is ClaudeCliMcpConfigurator(see line 307). The defensive check is harmless but adds unnecessary code.Consider removing the unreachable branch for clarity:
🔎 Proposed simplification
Task.Run(() => { - // For Claude CLI configurator, use thread-safe version with captured values - if (client is ClaudeCliMcpConfigurator claudeConfigurator) - { - claudeConfigurator.CheckStatusWithProjectDir(projectDir, useHttpTransport, attemptAutoRewrite: false); - } - else - { - MCPServiceLocator.Client.CheckClientStatus(client, attemptAutoRewrite: false); - } + // Use thread-safe version with captured main-thread values + ((ClaudeCliMcpConfigurator)client).CheckStatusWithProjectDir(projectDir, useHttpTransport, attemptAutoRewrite: false); }).ContinueWith(t =>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
🧰 Additional context used
🧠 Learnings (5)
📚 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/MCPForUnityEditorWindow.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
🧬 Code graph analysis (3)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
RefreshSelectedClient(292-303)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (4)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
ClaudeCliMcpConfigurator(328-602)ClaudeCliMcpConfigurator(330-330)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(51-56)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (5)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (1)
McpStatus(30-30)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Models/McpClient.cs (1)
SetStatus(42-54)MCPForUnity/Editor/Helpers/McpLog.cs (4)
Error(48-51)McpLog(7-52)Warn(43-46)Info(37-41)
⏰ 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (11)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (5)
337-340: LGTM: Clean delegation to thread-safe internal method.The refactoring separates the public interface from the implementation that supports background thread calls. This maintains backward compatibility while enabling the new async status check flow.
363-370: Thread safety concern:Application.platformaccessed unconditionally.
Application.platformis accessed without a null-check forprojectDir, meaning this code path runs regardless of whether we're on a background thread. UnlikeApplication.dataPath,Application.platformis thread-safe in Unity (it's a static readonly value cached at startup), so this is actually safe.
384-416: Transport mismatch detection logic is correct and well-structured.The transport validation:
- Runs
mcp listto check if UnityMCP exists- Runs
mcp get UnityMCPto retrieve registration details- Parses output for "Type: http" or "Type: stdio" (case-insensitive)
- Compares against current preference and sets error status on mismatch
The logic correctly handles the case where neither http nor stdio is detected (falls through to
Configuredstatus).
489-512: LGTM: Clean registration flow with proper cleanup.The registration logic:
- Checks for existing registration and removes it first
- Logs the cleanup action for debugging
- Handles removal failure gracefully (warns but continues)
- Sets status immediately after successful registration
This addresses the stale registration issue and avoids blocking CheckStatus calls.
553-555: LGTM: Status is set directly, avoiding blocking call.The comment clarifies the intent - status is already set, so no additional CheckStatus call is needed.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)
58-58: LGTM: New event follows established pattern.The
OnTransportChangedevent follows the sameActionpattern as the existingOnManualConfigUpdateRequestedevent on line 57.
119-119: LGTM: Event invocation placed correctly in the callback flow.The
OnTransportChangedevent is invoked afterOnManualConfigUpdateRequested, which ensures the manual configuration is updated before the client config section refreshes with the new transport setting. This ordering prevents stale data in the refresh.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
178-179: LGTM: Transport change triggers immediate client refresh.The subscription correctly uses
forceImmediate: trueto bypass the 45-second refresh debounce, ensuring the UI immediately reflects the transport change. The null-conditional operator safely handles the case whereclientConfigSectionmight not be initialized.Note: The event subscription is not explicitly unsubscribed in
OnDisable. However, sinceconnectionSectionis an instance field that gets recreated inCreateGUI, andguiCreatedis reset tofalseinOnDisable, the subscription lifetime is tied to the window's UI lifecycle, which appears acceptable for this Unity editor window pattern.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (3)
9-9: LGTM: Necessary import for EditorPrefKeys constant.The import is required for accessing
EditorPrefKeys.UseHttpTransporton line 343.
292-303: LGTM: Well-designed forceImmediate parameter.The logic
forceImmediate || client is not ClaudeCliMcpConfiguratorensures:
- Non-Claude clients always refresh immediately (synchronous and fast)
- Claude CLI clients only force immediate when explicitly requested (e.g., transport change)
This addresses the PR objective of immediate status updates on transport changes while preserving the async behavior for Claude CLI during normal operation.
336-356: LGTM: Thread-safe status check implementation.The implementation correctly:
- Guards against concurrent refreshes with
statusRefreshInFlight- Captures main-thread-only values (
Application.dataPath,EditorPrefs) beforeTask.Run- Routes Claude CLI to the thread-safe
CheckStatusWithProjectDirmethod- Falls back to standard
CheckClientStatusfor other clients (defensive, though this branch shouldn't be reached given the routing inRefreshClientStatus)
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.
Contributor
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/Clients/McpClientConfiguratorBase.cs (1)
373-393: RemoveApplication.platformaccess from thread-safe method or pass platform as parameter.The method
CheckStatusWithProjectDiris documented to be called from background threads, yet it directly accessesApplication.platformon lines 374 and 378. Unity's documentation explicitly states that the Unity API is not thread-safe and should only be accessed from the main thread. To maintain thread-safety, capture the platform value in the main-thread caller (CheckStatus) and pass it as a parameter toCheckStatusWithProjectDir, similar to howprojectDiranduseHttpTransportare already handled.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
395-427: Transport mismatch detection relies on CLI output format.The parsing of "Type: http" and "Type: stdio" strings (lines 409-410) assumes a specific human-readable output format from
claude mcp get. This is consistent with the PR objective to parse human-readable output rather than JSON. However, CLI output formats can change between versions.Consider adding a fallback or logging when neither transport type is detected, to handle potential format changes gracefully:
🔎 Suggested defensive handling
bool registeredWithHttp = getStdout.Contains("Type: http", StringComparison.OrdinalIgnoreCase); bool registeredWithStdio = getStdout.Contains("Type: stdio", StringComparison.OrdinalIgnoreCase); + +// If neither transport detected, log and assume configured (graceful degradation) +if (!registeredWithHttp && !registeredWithStdio) +{ + McpLog.Warn($"Could not determine transport type from Claude CLI output. Assuming configured."); + client.SetStatus(McpStatus.Configured); + return client.status; +} // Check for transport mismatch
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
📚 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/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
ClaudeCliMcpConfigurator(328-613)ClaudeCliMcpConfigurator(330-330)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (5)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (1)
McpStatus(30-30)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-49)MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)MCPForUnity/Editor/Models/McpClient.cs (1)
SetStatus(42-54)MCPForUnity/Editor/Helpers/McpLog.cs (4)
Error(48-51)McpLog(7-52)Warn(43-46)Info(37-41)
⏰ 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/Windows/Components/ClientConfig/McpClientConfigSection.cs (2)
292-303: LGTM - API extension for forced refresh.The
forceImmediateparameter enables transport-change-triggered refreshes. The logic correctly forces immediate refresh for non-Claude CLI clients (which are fast synchronous checks) while respecting the parameter for Claude CLI.
336-350: Thread-safety fix looks correct.Capturing
Application.dataPathandEditorPrefs.GetBool()on the main thread beforeTask.Runproperly addresses the threading hazard. The cast toClaudeCliMcpConfiguratoron line 349 is safe since this method is only called when the guard on line 307 passes.One minor observation: consider whether the
attemptAutoRewrite: falseon line 350 is intentional for async checks. The main-threadCheckStatus()defaults totrue, so background refreshes won't auto-rewrite configurations. This seems correct for status-only polling but worth confirming.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (4)
337-347: Thread-safety design is well-structured.The separation of concerns is clean:
CheckStatus()handles main-thread API access and delegates to the thread-safeCheckStatusWithProjectDir(). The XML doc correctly documents the threading contract.
349-371: Thread-safe overload with proper contract enforcement.The
ArgumentNullExceptionon line 370 codifies the thread-safety requirement at runtime, preventing misuse. The XML docs clearly state the threading contract.
500-522: Cleanup-before-register pattern ensures transport consistency.The change to always remove existing registration before re-registering (lines 500-510) correctly addresses stale registration handling. The warning on line 508 appropriately continues despite removal failure. Setting status to
Configuredimmediately (line 522) avoids the UI blocking issue.
564-566: Consistent with Register() pattern.The comment clarifies the intentional removal of the blocking
CheckStatus()call, matching the pattern established inRegister().
Collaborator
Author
|
Closing, it's included in #499 |
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
This PR fixes critical issues with Claude Code registration in HTTP mode, including UI blocking,
thread safety violations, and transport mismatch detection.
Issues Fixed
1. UI Blocking During Registration
The registration process was blocking the Unity Editor UI due to synchronous status checks after
registration completed. Users would experience freezing when clicking the Register button.
Fix: Removed blocking
CheckStatus()call after registration. Status is now set immediately toConfigured, with async verification running in the background.2. Thread Safety Violations
The async status check was calling Unity main-thread-only APIs (
Application.dataPathandEditorPrefs.GetBool()) from background threads, causing errors:GetBool can only be called from the main thread
get_dataPath can only be called from the main thread
Fix: Capture these values on the main thread before spawning async tasks, then pass them to
background threads via closure.
3. Transport Mismatch Not Detected
When switching between HTTP and stdio transport modes, the Claude Code registration status wasn't
immediately updated, showing green (configured) even when the registration was invalid.
Fix: Force immediate status check when transport mode changes, instead of waiting for the
45-second refresh interval.
4. Transport Validation Parsing
The Claude CLI output format wasn't being parsed correctly to detect HTTP vs stdio registration.
Fix: Updated to parse the correct CLI output format (
Type: http/Type: stdio).5. Stale Registration on Transport Change
When transport mode changed, the old registration wasn't being removed before re-registering.
Fix: Always remove existing UnityMCP registration before re-registering to ensure transport mode
is up-to-date.
Summary by Sourcery
Resolve Claude CLI-based MCP client registration and status issues, including transport detection, async status checks, and editor responsiveness.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.