Fix Claude Windows config and CLI status refresh #412
Conversation
WalkthroughAdds OS-aware uvx discovery and PATH enumeration, changes uvx/stdio command-arg construction including an optional Windows cmd.exe shim, introduces debounced asynchronous per-client status refresh, sets a StripEnvWhenNotRequired flag for Claude Desktop, and adds a unit test verifying absolute uvx override behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
155-192: Helper methods are solid; consider avoiding the hard‑coded client name
BuildUvxArgsandResolveCmdPathare straightforward and defensive (ComSpec → system32\cmd.exe → fallback), andShouldUseWindowsCmdShimkeeps the workaround tightly bound to Windows editor. The only minor nit is the magic string"Claude Desktop"; if you expect this client name to be reused elsewhere, consider centralizing it (e.g., a constant on the configurator or client asset) to avoid subtle breakage if the asset name ever changes.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
319-359: Consider handling background status‑check failures explicitlyThe
Task.Run/ContinueWithpath runsCheckClientStatusoff the main thread, which is good for responsiveness, but any exceptions thrown there will currently be unobserved and only surface via Unity’s unhandled Task exception behavior (if at all). It’s worth (a) double‑checking that the Claude configurator’sCheckStatusimplementation doesn’t touch Unity/Editor APIs that must run on the main thread, and (b) optionally loggingt.Exceptionin the continuation so background failures are visible when diagnosing status issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(3 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(5 hunks)
🧰 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/Helpers/ConfigJsonBuilder.csMCPForUnity/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/Helpers/ConfigJsonBuilder.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
System(404-502)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (4)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (1)
McpStatus(30-30)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-85)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(51-56)MCPForUnity/Editor/Models/McpClient.cs (1)
GetStatusDisplayString(23-39)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
83-98: Windows cmd shim for Claude Desktop stdio looks correct and narrowly scopedThe uvx arg construction (
BuildUvxArgs) and the cmd.exe wrapping forClaude DesktoponRuntimePlatform.WindowsEditorpreserve the expecteduvx --from … mcp-for-unity --transport stdiolayout while only altering thecommand/argspair for that specific client. This matches the PR goal and should leave macOS/Linux and other clients’ stdio configs unchanged.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
42-45: Async, debounced status refresh flow aligns well with the UX goalsThe per‑client
lastStatusChecks+ 45sStatusRefreshInterval, combined withstatusRefreshInFlight, give you a clean debounce that avoids hammeringCheckClientStatuswhile still forcing a refresh immediately after configuration or explicit “Refresh”. The Claude‑specific path updates the UI to “Checking…” while the background check runs and safely re-applies the final status only if that client is still selected, which should eliminate the previous freeze/loop without introducing obvious threading hazards.Also applies to: 106-113, 185-223, 291-299, 302-369, 371-413
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1)
170-202: Solid coverage of Claude Desktop uvx override; consider using the service API instead of raw EditorPrefsThe test correctly validates that a non-HTTP Claude Desktop config:
- Picks up the absolute
uvxoverride ascommand, and- Omits the
envblock whenStripEnvWhenNotRequiredis true, while still passing the shared transport assertions.One minor thought: you’re setting
EditorPrefKeys.UvxPathOverridedirectly. Wiring this throughPathResolverService.SetUvxPathOverride(and/or a small helper) would:
- Keep the test aligned with the public override contract, and
- Avoid duplicating knowledge of the pref key here.
Not strictly necessary for correctness, but it would make the test more robust to future changes in how overrides are stored.
MCPForUnity/Editor/Services/PathResolverService.cs (1)
2-205: Uvx system discovery is robust; consider slightly broader Windows candidate coverageThe new
GetUvxPath+ResolveUvxFromSystem/EnumerateUvxCandidatesflow looks sound:
- Pref override is honored first (matching the new test expectations).
- Discovery checks common home, system, and PATH-based locations per OS and returns a concrete absolute path when found.
- All failures cleanly fall back to the previous behavior of returning the bare
"uvx"command, so this is a safe upgrade in practice.One optional enhancement for Windows:
EnumerateUvxCandidatescurrently probesuvx.exeanduvxunder PATH entries. If uvx is ever installed as a script likeuvx.cmd/uvx.batthat only lives on PATH,File.Existswill miss it, and you’ll fall back to"uvx"instead of an absolute path. That still works functionally, but you could tighten detection by also checking for those extensions when constructing Windows PATH candidates.Not a blocker, just a possible follow-up refinement if you run into such installs in the wild.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs(1 hunks)MCPForUnity/Editor/Services/PathResolverService.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/Services/PathResolverService.cs
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (3)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-40)MCPForUnity/Editor/Models/McpClient.cs (1)
McpClient(5-55)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
JObject(31-41)JObject(147-153)
🔇 Additional comments (1)
MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs (1)
18-19: Claude Desktop flags look consistent with intended behaviorSetting
SupportsHttpTransport = falseandStripEnvWhenNotRequired = truealigns with Claude Desktop being stdio-only and the new tests that assert noenvblock is emitted when not needed. No issues from this change in this file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
102-107: Consider removing redundant property removals.Lines 102-103 explicitly remove both "url" and "serverUrl", and then lines 104-107 iterate
urlPropsToRemoveto remove one of these properties again. Since both properties are already removed by lines 102-103, the foreach loop on lines 104-107 is redundant.Apply this diff to remove the redundancy:
// Remove url/serverUrl if they exist from previous config if (unity["url"] != null) unity.Remove("url"); if (unity["serverUrl"] != null) unity.Remove("serverUrl"); -foreach (var prop in urlPropsToRemove) -{ - if (unity[prop] != null) unity.Remove(prop); -}MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
319-364: LGTM! Async refresh implementation with good in-flight protection.The asynchronous background refresh for Claude CLI clients successfully moves the blocking
CheckClientStatuscall off the main thread usingTask.Runand marshals results back viaEditorApplication.delayCall. The in-flight tracking on line 341-343 prevents duplicate concurrent refreshes for the same client.Minor improvement opportunity: Consider surfacing async errors to the UI.
Lines 351-354 log exceptions but don't update the UI to reflect the error state. When
CheckClientStatusfails, the UI remains showing the cached status (or "Checking..." if there was no cached status), which might be confusing. Consider setting an error status on the client when the task faults, so users are aware the check failed.Example enhancement:
.ContinueWith(t => { if (t.IsFaulted && t.Exception != null) { McpLog.Error($"Failed to refresh Claude CLI status: {t.Exception.GetBaseException().Message}"); + client.SetStatus(McpStatus.Error, "Status check failed"); } EditorApplication.delayCall += () =>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs(1 hunks)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(3 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(5 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs(2 hunks)
🧰 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/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.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/Helpers/ConfigJsonBuilder.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
🧬 Code graph analysis (4)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (3)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs (2)
ClaudeDesktopConfigurator(10-55)ClaudeDesktopConfigurator(14-23)MCPForUnity/Editor/Models/McpClient.cs (1)
McpClient(5-55)
MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs (1)
MCPForUnity/Editor/Models/McpClient.cs (1)
McpClient(5-55)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (4)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (2)
GetConfigureActionLabel(24-24)McpStatus(30-30)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (8)
ClaudeCliMcpConfigurator(328-554)ClaudeCliMcpConfigurator(330-330)GetConfigureActionLabel(32-32)GetConfigureActionLabel(333-333)McpStatus(35-35)McpStatus(89-191)McpStatus(232-289)McpStatus(337-392)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)MCPForUnity/Editor/Models/McpClient.cs (1)
GetStatusDisplayString(23-39)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (2)
MCPForUnity/Editor/Services/PathResolverService.cs (2)
SetUvxPathOverride(208-222)ClearUvxPathOverride(240-243)MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
JObject(32-42)JObject(148-154)
🔇 Additional comments (10)
MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs (1)
12-21: LGTM!The introduction of the
ClientNameconstant promotes consistency across the codebase and enables the Windows cmd shim detection logic inConfigJsonBuilder.cs. The addition ofStripEnvWhenNotRequired = truealigns with the test expectations and the PR's stated goal of environment-stripping for Claude Desktop configurations.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1)
171-203: LGTM! Good test coverage for the absolute uvx path override.The test correctly validates that when a uvx path override is provided, the generated Claude Desktop configuration uses the absolute path and strips the env block as expected. The try-finally cleanup ensures the override is cleared even if assertions fail.
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (4)
84-99: LGTM! Windows cmd shim logic is correctly implemented.The conditional cmd.exe wrapping for Claude Desktop on Windows correctly addresses the "Git operation failed" issue mentioned in the PR objectives. The implementation properly constructs the command and arguments for both the Windows shim path and the standard path.
156-170: LGTM! Clean argument construction.The
BuildUvxArgshelper cleanly assembles the uvx arguments with optional--fromand mandatory--transport stdioflags. The logic correctly prepends the--fromand URL when available.
172-181: LGTM! Platform and client detection is correct.The Windows cmd shim detection properly checks for the Windows editor platform and uses case-insensitive comparison with the
ClaudeDesktopConfigurator.ClientNameconstant. The null check prevents issues when client is not provided.
183-193: LGTM! Reasonable cmd.exe resolution with fallbacks.The
ResolveCmdPathmethod provides a sensible resolution chain: ComSpec environment variable → System32\cmd.exe → bare "cmd.exe" string. The fallback to "cmd.exe" is acceptable since it should always be available in PATH on Windows systems.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (4)
42-44: LGTM! Good addition of refresh tracking infrastructure.The per-client status check tracking with
lastStatusChecksandstatusRefreshInFlightcombined with the 45-second throttle interval effectively addresses the UI freeze loop issue described in the PR objectives.
302-317: LGTM! Clear separation of Claude CLI and standard refresh paths.The routing logic correctly distinguishes between Claude CLI clients (which need async background checks) and standard clients (which can use synchronous checks when needed). The force-immediate flag provides explicit control for post-configuration verification.
366-374: LGTM! Simple and effective throttling logic.The 45-second refresh interval check correctly prevents excessive status polling while ensuring reasonably fresh status information.
376-418: LGTM! Comprehensive UI status application with "Checking..." feedback.The
ApplyStatusToUimethod properly handles the "Checking..." state (lines 388-395) and applies appropriate CSS classes and button labels based on client status. The reference equality check on line 381 ensures UI updates only apply to the currently selected client.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
329-330: RedundantforceImmediatecheck.The
forceImmediatein theneedsRefreshexpression on line 330 is dead code sinceforceImmediate: truereturns early at line 327. This doesn't affect correctness but adds confusion.- bool needsRefresh = forceImmediate || !hasStatus || ShouldRefreshClient(client); + bool needsRefresh = !hasStatus || ShouldRefreshClient(client);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs(3 hunks)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.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/Helpers/ConfigJsonBuilder.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/Helpers/ConfigJsonBuilder.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
uvxPath(169-176)MCPForUnity/Editor/Clients/Configurators/ClaudeDesktopConfigurator.cs (3)
IList(25-31)ClaudeDesktopConfigurator(10-55)ClaudeDesktopConfigurator(14-23)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (5)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (3)
System(39-39)McpStatus(30-30)GetConfigureActionLabel(24-24)MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (8)
McpClientConfiguratorBase(17-80)McpClientConfiguratorBase(21-24)McpStatus(35-35)McpStatus(89-191)McpStatus(232-289)McpStatus(337-392)GetConfigureActionLabel(32-32)GetConfigureActionLabel(333-333)MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
CheckClientStatus(51-56)MCPForUnity/Editor/Services/IClientConfigurationService.cs (1)
CheckClientStatus(30-30)MCPForUnity/Editor/Models/McpClient.cs (1)
GetStatusDisplayString(23-39)
🔇 Additional comments (11)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (6)
42-44: LGTM! Per-client status tracking with appropriate interval.The debouncing mechanism with a 45-second interval is a reasonable choice to prevent excessive CLI invocations when refocusing Unity. Thread safety is maintained since all collection modifications occur on the main thread (either directly or via
EditorApplication.delayCall).
106-113: LGTM!Clean delegation to the new
RefreshClientStatusmethod while preserving the bounds check.
221-222: LGTM!Clearing the last check timestamp and forcing immediate refresh ensures the UI updates right away after configuration, eliminating the previous "freeze" behavior.
302-317: LGTM!Clean separation between Claude CLI (async) and other clients (sync). Non-Claude clients don't have the blocking CLI issue, so synchronous handling is appropriate.
346-374: LGTM! Async pattern correctly marshals back to main thread.The
Task.Run/ContinueWith/EditorApplication.delayCallpattern properly offloads the blocking CLI call and marshals the result back to Unity's main thread. Exception handling captures and logs faults appropriately.
388-430: LGTM! Centralized UI state management.The method properly guards against stale updates for non-selected clients, cleanly manages CSS class states, and handles the "Checking..." intermediate state. Using
StyleKeyword.Nullto reset colors is the correct UIElements approach.MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (5)
3-6: LGTM!The new imports support the Windows cmd shim logic and path resolution features.
152-166: LGTM!The argument construction correctly builds the uvx command line:
[--from URL] package --transport stdio. The order matches uvx's expected syntax.
168-177: LGTM!The Windows cmd shim is correctly scoped to only Claude Desktop on Windows, matching the PR objective. Using the
ClaudeDesktopConfigurator.ClientNameconstant ensures consistency with client registration.
179-189: LGTM!Robust cmd.exe resolution with appropriate fallbacks:
ComSpec→System32\cmd.exe→ barecmd.exe. TheEnvironment.SpecialFolder.Systemcorrectly resolves to System32 on Windows.
86-99: The cmd.exe /c argument handling properly supports paths with spaces through the JSON array format.The JSON array format used by MCP clients (like Claude Desktop running on Node.js) passes each argument as a separate element. When Node.js spawns
cmd.exewith["/c", "C:\Program Files\uvx.exe", ...], each array element becomes a separate argv entry, so cmd.exe receives the path as a distinct argument without needing quotes. The/cflag then reads this argument directly, and spaces within individual argv elements are handled correctly by the OS process creation API.Quoting the uvxPath would be incorrect here, as it would cause cmd.exe to receive the literal quoted string rather than the intended path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
346-374: Consider adding a timeout for the async status check.The
Task.Runcall has no timeout, which could cause the UI to remain in the "Checking..." state indefinitely if the CLI process hangs or becomes unresponsive. While error handling is present, it won't catch a hung process.Consider wrapping the check in a timeout:
- Task.Run(() => - { - MCPServiceLocator.Client.CheckClientStatus(client, attemptAutoRewrite: false); - }).ContinueWith(t => + var statusTask = Task.Run(() => + { + MCPServiceLocator.Client.CheckClientStatus(client, attemptAutoRewrite: false); + }); + + var timeoutTask = Task.Delay(TimeSpan.FromSeconds(30)); + Task.WhenAny(statusTask, timeoutTask).ContinueWith(_ => { bool faulted = false; string errorMessage = null; - if (t.IsFaulted && t.Exception != null) + + if (timeoutTask.IsCompleted && !statusTask.IsCompleted) + { + errorMessage = "Status check timed out after 30 seconds"; + McpLog.Error($"Claude CLI status check timed out"); + faulted = true; + } + else if (statusTask.IsFaulted && statusTask.Exception != null) { - var baseException = t.Exception.GetBaseException(); + var baseException = statusTask.Exception.GetBaseException(); errorMessage = baseException?.Message ?? "Status check failed"; McpLog.Error($"Failed to refresh Claude CLI status: {errorMessage}"); faulted = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🔇 Additional comments (6)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (6)
7-7: LGTM!The
System.Threading.Tasksimport is necessary for the asynchronous status refresh implementation.
106-113: LGTM!The delegation to
RefreshClientStatuscentralizes the status refresh logic and enables throttling.
211-232: LGTM!The immediate status refresh after configuration ensures the UI reflects the updated state without delay, improving user experience.
302-317: LGTM!The throttling logic prevents excessive status checks while allowing immediate updates when needed. The separation of Claude CLI handling is appropriate given its async requirements.
378-386: LGTM!The interval-based refresh logic is straightforward and correct.
388-430: LGTM!The UI update logic correctly handles the "Checking..." state and applies appropriate styling based on client status. The
ReferenceEqualscheck ensures UI updates only apply to the currently selected client.
* 'main' of https://github.com/CoplayDev/unity-mcp: chore: bump version to 8.1.4 Fix Claude Windows config and CLI status refresh (CoplayDev#412) chore: bump version to 8.1.3 fix: restrict fastmcp version to avoid potential KeyError (CoplayDev#411) chore: bump version to 8.1.2 Revert project script change chore: bump version to 8.1.1 Fix CLI entry point path in pyproject.toml (CoplayDev#407) Fix manage prefabs (CoplayDev#405) Remove automatic version bumping from README files and switch to unversioned git URLs chore: bump version to 8.1.0 Add distribution settings for Asset Store vs git defaults (CoplayDev#404) Add CodeBuddy CLI configurator (CoplayDev#403) Fix stdio reloads (CoplayDev#402) Simplify MCP client configs (CoplayDev#401) chore: bump version to 8.0.1
1 participant
cmd.exe /c …, matching the verified workaround for the “Git operation failed” issue. macOS/Linux configurations remain unchanged.claude mcp listruns, and the UI updates immediately after registration/unregistration, eliminating the UI “freeze” loop when refocusing Unity.And: Added an absolute-path discovery pass for uvx so Claude Desktop configs no longer rely on GUI PATH. We now look in typical install folders for macOS, Linux, and Windows (including every entry already on PATH) before writing the command; if the binary is available anywhere standard, the generated config points to it directly. This keeps the flow automatic for fresh installs while still falling back to the bare command for CI or machines that lack uvx.
Tested:
uvxremote call; confirmed JSON now uses the cmd wrapper.Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.