[FEATURE] Custom Tool Fix and Add inspection window for all the tools #414
Conversation
* Update the .Bat file to include runtime folder * Fix the inconsistent EditorPrefs variable so the GUI change on Script Validation could cause real change.
String to Int for consistency
Allows users to generate/compile codes during Playmode
Upload the unity_claude_skill that can be uploaded to Claude for a combo of unity-mcp-skill.
1. Fix Original Roslyn Compilation Custom Tool to fit the V8 standard 2. Add a new panel in the GUI to see and toggle/untoggle the tools. The toggle feature will be implemented in the future, right now its implemented here to discuss with the team if this is a good feature to add; 3. Add few missing summary in certain tools
This reverts commit ae8cfe5.
This reverts commit f423c2f.
Merge and use the previous PR's solution to solve the windows issue.
Fix the layout problem of manage_script in the panel
WalkthroughThe PR refactors the Roslyn runtime compilation response types to use ErrorResponse and SuccessResponse wrappers, removes the Python runtime_compilation_tool.py module, adds tool enablement/discovery functionality via EditorPrefs, introduces a new McpToolsSection UI component for tool management, refactors the editor window into a tabbed Settings/Tools layout, and adds a new BatchExecute command for processing up to 25 MCP commands with optional parallelism and fail-fast logic. 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, 1 inconclusive)
✅ Passed checks (1 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: 1
🧹 Nitpick comments (7)
MCPForUnity/Editor/Windows/Components/Common.uss (1)
472-487: Dark theme overrides look good.The color adjustments for
.tool-tag,.tool-item, and descriptions ensure proper contrast in dark mode.Minor: Line 487 has an extra blank line that could be removed for consistency.
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
29-34: Consider adding null guard forrootparameter.If
rootis passed as null,Root.Q<T>()calls inCacheUIElements()will throw aNullReferenceException. Consider adding a guard clause.public McpToolsSection(VisualElement root) { - Root = root; + Root = root ?? throw new ArgumentNullException(nameof(root)); CacheUIElements(); RegisterCallbacks(); }MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (3)
1-20: Remove duplicate using statements.Lines 5-9 are duplicated at lines 15-19. Remove the duplicate declarations to improve code clarity.
Apply this diff to remove the duplicates:
using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; -using MCPForUnity.Editor.Helpers; -using MCPForUnity.Editor.Services; -using MCPForUnity.Editor.Windows.Components.ClientConfig; -using MCPForUnity.Editor.Windows.Components.Connection; -using MCPForUnity.Editor.Windows.Components.Settings; using UnityEditor; using UnityEditor.UIElements; using UnityEngine; using UnityEngine.UIElements; using MCPForUnity.Editor.Constants; using MCPForUnity.Editor.Helpers; using MCPForUnity.Editor.Services; using MCPForUnity.Editor.Windows.Components.Settings; using MCPForUnity.Editor.Windows.Components.Connection; using MCPForUnity.Editor.Windows.Components.ClientConfig; using MCPForUnity.Editor.Windows.Components.Tools;
48-88: Consolidate duplicate window-closing methods.Both
CloseAllWindows()(lines 48-55) andCloseAllOpenWindows()(lines 69-88) perform the same task. The latter has better error handling and is public. Consider removingCloseAllWindows()and usingCloseAllOpenWindows()throughout, or makeCloseAllWindows()delegate to the more robust implementation.Apply this diff to consolidate:
-internal static void CloseAllWindows() -{ - var windows = OpenWindows.Where(window => window != null).ToArray(); - foreach (var window in windows) - { - window.Close(); - } -} - public static void ShowWindow() { var window = GetWindow<MCPForUnityEditorWindow>("MCP For Unity"); window.minSize = new Vector2(500, 600); } // Helper to check and manage open windows from other classes public static bool HasAnyOpenWindow() { return OpenWindows.Count > 0; } public static void CloseAllOpenWindows()
267-336: Consider consistent visibility approach.The code removes the "hidden" CSS class from panels (lines 272-273) but then uses
DisplayStyle.None/Flexto control visibility (lines 324-329). This mixed approach could lead to style conflicts if the "hidden" class also modifies display properties. Consider using one consistent method (preferablyDisplayStylefor programmatic control).If the "hidden" class is not needed, remove lines 272-273:
private void SetupTabs() { settingsTabToggle = rootVisualElement.Q<ToolbarToggle>("settings-tab"); toolsTabToggle = rootVisualElement.Q<ToolbarToggle>("tools-tab"); - settingsPanel?.RemoveFromClassList("hidden"); - toolsPanel?.RemoveFromClassList("hidden"); - if (settingsTabToggle != null)MCPForUnity/Editor/Services/ToolDiscoveryService.cs (2)
79-94: Optional: Remove redundant default parameter.On Line 89, the default parameter
trueinEditorPrefs.GetBool(key, true)is never used because the call is inside anif (HasKey)block. Consider simplifying toEditorPrefs.GetBool(key).if (EditorPrefs.HasKey(key)) { - return EditorPrefs.GetBool(key, true); + return EditorPrefs.GetBool(key); }
354-404: Consider a more robust approach for summary extraction.The regex-based parsing of XML documentation comments is fragile and may fail with different code formatting styles:
- Line 378: The class-specific pattern assumes specific ordering of comments, attributes, and keywords
- Line 383: The fallback pattern matches any
<summary>in the file, which could extract documentation from a different symbol- Different indentation, attribute placement, or comment styles may not match the patterns
While the caching and error handling ensure failures won't break the system, you might see inconsistent results across different coding styles.
Consider alternatives:
- Use Roslyn's syntax API to parse the source and extract the documentation comment structurally
- Accept that XML doc comments are best-effort and document the expected format
- Investigate Unity's built-in documentation extraction APIs (if available)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CustomTools/RoslynRuntimeCompilation/ManageRuntimeCompilation.cs(17 hunks)CustomTools/RoslynRuntimeCompilation/RoslynRuntime.md(0 hunks)CustomTools/RoslynRuntimeCompilation/runtime_compilation_tool.py(0 hunks)MCPForUnity/Editor/Constants/EditorPrefKeys.cs(1 hunks)MCPForUnity/Editor/Services/IToolDiscoveryService.cs(2 hunks)MCPForUnity/Editor/Services/ToolDiscoveryService.cs(6 hunks)MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs(1 hunks)MCPForUnity/Editor/Tools/ExecuteMenuItem.cs(1 hunks)MCPForUnity/Editor/Tools/ManageScript.cs(2 hunks)MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Common.uss(2 hunks)MCPForUnity/Editor/Windows/Components/Tools.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs(1 hunks)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs.meta(1 hunks)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxml(1 hunks)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxml.meta(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(6 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml(1 hunks)
💤 Files with no reviewable changes (2)
- CustomTools/RoslynRuntimeCompilation/runtime_compilation_tool.py
- CustomTools/RoslynRuntimeCompilation/RoslynRuntime.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Constants/EditorPrefKeys.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/Constants/EditorPrefKeys.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxml.metaMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs.metaMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Windows/Components/Tools.metaMCPForUnity/Editor/Services/ToolDiscoveryService.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/Constants/EditorPrefKeys.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs.metaMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxmlMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs.metaMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxmlMCPForUnity/Editor/Tools/ExecuteMenuItem.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/Tools/ManageScript.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csCustomTools/RoslynRuntimeCompilation/ManageRuntimeCompilation.csMCPForUnity/Editor/Services/ToolDiscoveryService.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/Windows/Components/Tools.meta
🧬 Code graph analysis (5)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(7-52)Info(37-41)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (3)
MCPForUnity/Editor/Services/IToolDiscoveryService.cs (7)
List(44-44)List(54-54)ToolMetadata(8-22)ToolMetadata(49-49)InvalidateCache(69-69)IsToolEnabled(59-59)SetToolEnabled(64-64)MCPForUnity/Editor/Services/ToolDiscoveryService.cs (8)
List(19-60)List(72-77)List(159-193)ToolMetadata(62-70)ToolMetadata(107-157)InvalidateCache(233-236)IsToolEnabled(79-94)SetToolEnabled(96-105)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-43)
MCPForUnity/Editor/Services/IToolDiscoveryService.cs (2)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
IsBuiltIn(268-268)MCPForUnity/Editor/Services/ToolDiscoveryService.cs (4)
ToolMetadata(62-70)ToolMetadata(107-157)IsToolEnabled(79-94)SetToolEnabled(96-105)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (3)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (1)
System(39-39)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (4)
McpToolsSection(16-269)McpToolsSection(29-34)VisualElement(146-203)Refresh(77-117)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-43)
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (3)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
IsBuiltIn(268-268)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-43)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)GetMcpPackageRootPath(41-80)
🔇 Additional comments (37)
MCPForUnity/Editor/Tools/ManageScript.cs (1)
27-53: LGTM! Documentation structure improved.The change properly separates the brief summary from the detailed Roslyn installation guide by moving the installation instructions into a
<remarks>section. This follows XML documentation best practices.MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
10-12: LGTM! Documentation added.The XML summary clearly describes the tool's purpose. This aligns with the PR objective to add missing documentation comments.
MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
13-15: LGTM! Documentation added.The XML summary clearly describes the prefab management capabilities. Consistent with the documentation improvements across the PR.
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uss (1)
1-37: LGTM! UI styling updated to support Tools panel.The CSS changes introduce layout structure for the new tabbed Tools UI:
- Root container updated with proper flex layout properties
- New tab toolbar and panel styling added
- Utility classes for visibility control
These styling changes align with the PR's goal to add a new GUI panel for tool management.
CustomTools/RoslynRuntimeCompilation/ManageRuntimeCompilation.cs (4)
20-26: LGTM! Documentation and attribute updated.The class documentation now clearly describes the runtime compilation benefits (avoiding domain reload), and the
McpForUnityToolattribute now includes thenameandDescriptionparameters, aligning with V8 standards.
145-230: LGTM! Response wrapper migration complete.All error and success responses have been consistently updated to use
ErrorResponseandSuccessResponse. The response structures maintain backward compatibility with the same data fields.
233-441: LGTM! Remaining response wrappers updated consistently.All methods (
ListLoadedAssemblies,GetAssemblyTypes,ExecuteWithRoslyn, history management methods) have been consistently updated to use the new response wrappers.
47-96: Assembly name sanitization is properly implemented and safe.The assembly name handling includes comprehensive protection against path traversal:
SanitizeAssemblyFileName()removes invalid filename characters usingPath.GetInvalidFileNameChars()Path.GetFullPath()normalizes the path (line 110)- Explicit validation ensures the resolved DLL path stays within
DynamicAssembliesPathviaStartsWith()check (lines 112-114)- If sanitization results in an empty string, it falls back to a generated name with ticks
The migration to
ErrorResponse/SuccessResponseis consistent throughout.MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
420-466: LGTM! Tool registration now respects enabled/disabled state.The change from
DiscoverAllTools()toGetEnabledTools()ensures that only enabled tools are registered with the bridge, supporting the new tool toggle functionality mentioned in the PR objectives. The added log message provides helpful visibility into the registration process.MCPForUnity/Editor/Windows/Components/Tools.meta (1)
1-8: LGTM! Unity meta file for new Tools folder.The meta file has been properly updated to reflect the new Tools folder asset structure with
folderAsset: yesandDefaultImporter. The GUID change is expected for new assets.MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxml.meta (1)
1-10: LGTM! Unity meta file for UXML asset.The meta file has been properly updated for the new
McpToolsSection.uxmlasset. The script reference withfileID: 13804is Unity's standard UXML importer, which is correct for UXML assets.MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs.meta (1)
1-11: LGTM!Standard Unity meta file with a unique GUID for the new
McpToolsSection.csasset.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.uxml (2)
4-7: LGTM!Tab toolbar structure is clean. The mutual exclusivity of toggles (radio-button behavior) will be managed by the C# code in
MCPForUnityEditorWindow.cs, which is appropriate.
8-13: LGTM!The two-panel layout with
tools-panelhidden by default is well-structured. Theflex-grow: 1ensures proper space allocation.MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
28-30: LGTM!New preference keys follow the established naming convention and are correctly placed. The prefix keys appropriately end with
.for tool name concatenation.MCPForUnity/Editor/Windows/Components/Common.uss (1)
296-364: LGTM!Well-structured CSS for the Tools section. Flexbox patterns are consistent with existing styles, and proper spacing/padding creates good visual hierarchy.
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.uxml (1)
1-15: LGTM!Clean UXML structure with appropriate placeholder text and action buttons. The note on line 11 provides helpful guidance about when tool changes take effect.
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (7)
16-26: LGTM!Class fields are well-organized with appropriate inline initialization. The
toolToggleMapdictionary provides efficient lookup for toggle synchronization.
46-72: Event handlers are not unsubscribed.The
clickedevent handlers are registered but never unsubscribed. In editor windows where the section may be recreated (e.g., on domain reload), this could accumulate handlers. However, since Unity editor windows typically manage element lifecycle internally and the handlers reference the same instance, this is likely acceptable for the current use case.If the
McpToolsSectionis intended to be disposed/recreated during the editor session, consider implementingIDisposableor a cleanup method to unregister callbacks.
77-117: LGTM!The
Refresh()method has solid logic flow: clear state, discover tools, update UI conditionally, and handle edge cases (no tools). The ordering strategy (built-in first, then alphabetical) provides good UX.
119-144: LGTM!Category building with foldout state persistence via
EditorPrefsis well-implemented. The count in the foldout title provides helpful context.
146-203: LGTM!Tool row creation is comprehensive:
- Toggle state correctly sourced from
IsToolEnabled()- Informative tags for default state, output type, and polling
- Conditional rendering for description and parameters
- Parameter summary format is clear and user-friendly.
215-235: LGTM!Bulk enable/disable logic is well-designed:
- Uses
SetValueWithoutNotify()to prevent recursive callbacks- Defers summary update until after all changes
- Defensively handles edge case where toggle is missing from map.
237-269: LGTM!Helper methods are clean and focused. The summary message clearly communicates the current enablement state to users.
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
195-209: LGTM!The Tools section loading includes proper error handling and defensive null checks. The warning message clearly indicates that tool configuration will be unavailable if the UXML fails to load.
338-356: LGTM!The health check scheduling correctly uses
EditorApplication.delayCallfor Unity editor context and includes proper null checks before awaiting async operations.MCPForUnity/Editor/Services/IToolDiscoveryService.cs (2)
16-21: LGTM!The new properties (
AssemblyName,AssetPath,IsBuiltIn) are well-named and appropriately typed to support tool discovery filtering and UI categorization.
51-64: LGTM!The new interface methods are well-documented and provide a clean API for tool enablement management. The method signatures are straightforward and follow C# naming conventions.
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (9)
5-6: LGTM!The new using statements are appropriate for the regex-based summary extraction and EditorPrefs key management functionality.
16-17: LGTM!The caching strategy is a good optimization to avoid repeated AssetDatabase queries and source code parsing.
47-47: LGTM!The preference initialization call is appropriately placed during tool discovery. Since discovery results are cached, this initialization happens once per session unless the cache is explicitly invalidated.
72-77: LGTM!The filtering logic is clean and correctly delegates to
IsToolEnabledfor the enablement check.
96-105: LGTM!The method correctly persists the enabled state with appropriate null checks.
125-150: LGTM!The metadata enrichment logic correctly populates the new fields (
AssemblyName,AssetPath,IsBuiltIn) and appropriately overrides descriptions for built-in tools when XML summaries are available. The exception handling covers potential issues with assembly access.
263-266: LGTM!The helper method provides a clean abstraction for constructing preference keys consistently.
268-316: LGTM!The asset path resolution is well-implemented with caching and error handling. The search filter appropriately includes the type name when available to narrow results, and the loop correctly identifies the exact matching script by comparing types.
318-352: LGTM!The dual classification strategy (asset path prefix and assembly name) provides robust built-in tool detection. The case-insensitive path comparison and path normalization ensure cross-platform compatibility.
| private void EnsurePreferenceInitialized(ToolMetadata metadata) | ||
| { | ||
| if (metadata == null || string.IsNullOrEmpty(metadata.Name)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string key = GetToolPreferenceKey(metadata.Name); | ||
| if (!EditorPrefs.HasKey(key)) | ||
| { | ||
| bool defaultValue = metadata.AutoRegister || metadata.IsBuiltIn; | ||
| EditorPrefs.SetBool(key, defaultValue); | ||
| return; | ||
| } | ||
|
|
||
| if (metadata.IsBuiltIn && !metadata.AutoRegister) | ||
| { | ||
| bool currentValue = EditorPrefs.GetBool(key, metadata.AutoRegister); | ||
| if (currentValue == metadata.AutoRegister) | ||
| { | ||
| EditorPrefs.SetBool(key, true); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify the preference override logic for built-in tools.
The logic in lines 253-260 may override user preferences unexpectedly:
- Line 255 has a redundant default parameter (same issue as Line 89)
- The condition
if (currentValue == metadata.AutoRegister)checks if the stored value isfalse(whenAutoRegisterisfalse), then forces it totrue - This doesn't distinguish between "user explicitly disabled the tool" vs "value was initialized to false based on AutoRegister"
If a user intentionally disables a built-in tool that has AutoRegister=false, this logic will silently re-enable it the next time tools are discovered. Consider whether this override is intended, or if it should only apply during a one-time migration scenario.
Potential approaches:
- Add a version flag to EditorPrefs to run this migration only once
- Skip this override if the tool is already in a disabled state
- Document that built-in tools cannot be permanently disabled
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/ToolDiscoveryService.cs around lines 238-261, the
preference-migration logic can unintentionally re-enable built-in tools users
explicitly disabled and uses a redundant default parameter; change the behavior
to use a one-time migration flag and remove the redundant default: implement an
EditorPrefs migration key (e.g., "ToolDiscovery_Migration_v1") to guard the
upgrade so the override runs only once, remove the unnecessary default argument
from EditorPrefs.GetBool, and only flip the stored value to true during that
guarded migration when metadata.IsBuiltIn && !metadata.AutoRegister and the
migration flag indicates the migration hasn't yet run; after performing the
migration set the migration flag so user preferences are not modified on
subsequent discoveries.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
97-105: Consider preserving additional exception details for debugging.Line 104 captures only
ex.Messagein the error response. For debugging batch failures, preserving the exception type and stack trace (at least in logs) could be helpful, especially when troubleshooting complex command sequences.Optionally log the full exception before recording the error:
catch (Exception ex) { + McpLog.Error($"Command '{toolName}' failed: {ex}"); failureCount++; commandResults.Add(new { success = false, tool = toolName, error = ex.Message });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/BatchExecute.cs(1 hunks)MCPForUnity/Editor/Tools/CommandRegistry.cs(1 hunks)Server/src/services/tools/batch_execute.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Tools/CommandRegistry.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/CommandRegistry.cs (1)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
Task(18-128)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
MCPForUnity/Editor/Tools/CommandRegistry.cs (2)
Task(257-279)CommandRegistry(37-437)
🪛 Ruff (0.14.7)
Server/src/services/tools/batch_execute.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
37-39: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Prefer TypeError exception for invalid type
(TRY004)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Prefer TypeError exception for invalid type
(TRY004)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
114-128: LGTM! Result aggregation is well-structured.The result aggregation correctly determines overall success, includes comprehensive metadata, and returns the appropriate response type. The
parallelApplied = falseflag (Line 121) properly reflects the sequential execution documented in the warning at Line 42.Server/src/services/tools/batch_execute.py (4)
16-29: LGTM! Tool definition is clear and well-documented.The tool decorator provides a clear description of batch behavior, and the function signature uses proper type annotations with helpful documentation strings. The parameter naming follows Python conventions (snake_case) and is correctly mapped to C# conventions (camelCase) when constructing the payload.
31-40: LGTM! Input validation is thorough.The validation logic properly checks for a non-empty command list and enforces the 25-command limit, which is consistent with the Unity-side implementation in
BatchExecute.cs(Line 16).
41-60: LGTM! Command normalization is robust.The normalization loop thoroughly validates each command's structure, provides clear error messages with indices for debugging, and ensures all commands have the expected format before forwarding to Unity. The use of
ValueErrorfor input validation is appropriate for this API boundary, despite Ruff's suggestion to preferTypeErrorfor type mismatches.
62-78: LGTM! Payload construction and transport are correct.The payload correctly maps Python snake_case parameters to C# camelCase conventions (e.g.,
fail_fast→failFastat Line 69), includes only provided optional parameters, and uses the command name"batch_execute"which matches the C# tool registration. Based on learnings, the command identifier consistency ensures proper routing to the Unity-side handler.
| /// Executes multiple MCP commands within a single Unity-side handler. Commands are executed sequentially | ||
| /// on the main thread to preserve determinism and Unity API safety. | ||
| /// </summary> | ||
| [McpForUnityTool("batch_execute", AutoRegister = false)] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "CommandRegistry.cs"Repository: CoplayDev/unity-mcp
Length of output: 155
🏁 Script executed:
wc -l MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 110
🏁 Script executed:
sed -n '75,100p' MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 1096
🏁 Script executed:
rg -n "RegisterCommandType|AutoRegister" MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 287
🏁 Script executed:
sed -n '113,180p' MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 2530
🏁 Script executed:
fd -t f "*.cs" | xargs rg -l "class McpForUnityToolAttribute"Repository: CoplayDev/unity-mcp
Length of output: 416
🏁 Script executed:
cat -n MCPForUnity/Editor/Tools/McpForUnityToolAttribute.csRepository: CoplayDev/unity-mcp
Length of output: 4308
The AutoRegister = false property is not respected by the discovery logic.
The [McpForUnityTool("batch_execute", AutoRegister = false)] attribute includes an AutoRegister property (defined in McpForUnityToolAttribute.cs line 31) intended to prevent tools from being automatically registered. However, CommandRegistry.AutoDiscoverCommands (line 85) only filters types by the presence of the McpForUnityToolAttribute without checking the AutoRegister property. This means the tool will be auto-discovered and registered regardless of the AutoRegister = false setting. Either remove the attribute to prevent auto-registration, or update the discovery logic to respect the AutoRegister flag.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/BatchExecute.cs line 13 and
CommandRegistry.AutoDiscoverCommands around line 85, the McpForUnityTool
attribute on the class sets AutoRegister = false but the discovery logic ignores
that flag; update AutoDiscoverCommands to read the McpForUnityToolAttribute
instance for each candidate type and only register tools when
attribute.AutoRegister is true (skip registration when AutoRegister is false).
Concretely: when enumerating types,
getCustomAttribute<McpForUnityToolAttribute>(), check for null and then check
attribute.AutoRegister before adding the command; alternatively, remove the
attribute from BatchExecute.cs if you intend the tool to be auto-registered.
| /// <summary> | ||
| /// Execute a command handler and return its raw result, regardless of sync or async implementation. | ||
| /// Used internally for features like batch execution where commands need to be composed. | ||
| /// </summary> | ||
| /// <param name="commandName">The registered command to execute.</param> | ||
| /// <param name="params">Parameters to pass to the command (optional).</param> | ||
| public static Task<object> InvokeCommandAsync(string commandName, JObject @params) | ||
| { | ||
| var handlerInfo = GetHandlerInfo(commandName); | ||
| var payload = @params ?? new JObject(); | ||
|
|
||
| if (handlerInfo.IsAsync) | ||
| { | ||
| if (handlerInfo.AsyncHandler == null) | ||
| { | ||
| throw new InvalidOperationException($"Async handler for '{commandName}' is not configured correctly"); | ||
| } | ||
|
|
||
| return handlerInfo.AsyncHandler(payload); | ||
| } | ||
|
|
||
| if (handlerInfo.SyncHandler == null) | ||
| { | ||
| throw new InvalidOperationException($"Handler for '{commandName}' does not provide a synchronous implementation"); | ||
| } | ||
|
|
||
| object result = handlerInfo.SyncHandler(payload); | ||
| return Task.FromResult(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add ConfigureAwait(true) for Unity main-thread safety.
The async path at Line 269 directly returns handlerInfo.AsyncHandler(payload) without applying .ConfigureAwait(true). This is inconsistent with Line 316 in CreateAsyncHandlerDelegate, which explicitly uses ConfigureAwait(true) to ensure continuation on Unity's main thread. Since Unity APIs require main-thread execution, the async handler invocation should also configure the awaiter to return to the synchronization context.
Apply this diff:
- return handlerInfo.AsyncHandler(payload);
+ return await handlerInfo.AsyncHandler(payload).ConfigureAwait(true);Note: This requires changing the method signature to async Task<object>:
- public static Task<object> InvokeCommandAsync(string commandName, JObject @params)
+ public static async Task<object> InvokeCommandAsync(string commandName, JObject @params)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Execute a command handler and return its raw result, regardless of sync or async implementation. | |
| /// Used internally for features like batch execution where commands need to be composed. | |
| /// </summary> | |
| /// <param name="commandName">The registered command to execute.</param> | |
| /// <param name="params">Parameters to pass to the command (optional).</param> | |
| public static Task<object> InvokeCommandAsync(string commandName, JObject @params) | |
| { | |
| var handlerInfo = GetHandlerInfo(commandName); | |
| var payload = @params ?? new JObject(); | |
| if (handlerInfo.IsAsync) | |
| { | |
| if (handlerInfo.AsyncHandler == null) | |
| { | |
| throw new InvalidOperationException($"Async handler for '{commandName}' is not configured correctly"); | |
| } | |
| return handlerInfo.AsyncHandler(payload); | |
| } | |
| if (handlerInfo.SyncHandler == null) | |
| { | |
| throw new InvalidOperationException($"Handler for '{commandName}' does not provide a synchronous implementation"); | |
| } | |
| object result = handlerInfo.SyncHandler(payload); | |
| return Task.FromResult(result); | |
| } | |
| /// <summary> | |
| /// Execute a command handler and return its raw result, regardless of sync or async implementation. | |
| /// Used internally for features like batch execution where commands need to be composed. | |
| /// </summary> | |
| /// <param name="commandName">The registered command to execute.</param> | |
| /// <param name="params">Parameters to pass to the command (optional).</param> | |
| public static async Task<object> InvokeCommandAsync(string commandName, JObject @params) | |
| { | |
| var handlerInfo = GetHandlerInfo(commandName); | |
| var payload = @params ?? new JObject(); | |
| if (handlerInfo.IsAsync) | |
| { | |
| if (handlerInfo.AsyncHandler == null) | |
| { | |
| throw new InvalidOperationException($"Async handler for '{commandName}' is not configured correctly"); | |
| } | |
| return await handlerInfo.AsyncHandler(payload).ConfigureAwait(true); | |
| } | |
| if (handlerInfo.SyncHandler == null) | |
| { | |
| throw new InvalidOperationException($"Handler for '{commandName}' does not provide a synchronous implementation"); | |
| } | |
| object result = handlerInfo.SyncHandler(payload); | |
| return Task.FromResult(result); | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/CommandRegistry.cs around lines 251 to 279, the
async branch returns handlerInfo.AsyncHandler(payload) directly and doesn't
ensure continuation on Unity's main thread; change the method signature to async
Task<object>, await the async handler with ConfigureAwait(true) (e.g. var result
= await handlerInfo.AsyncHandler(payload).ConfigureAwait(true); return result;),
leaving the synchronous path unchanged and preserving the same exception checks.
Tested object generation/modification with batch and it works perfectly! We should push and let users test for a while and see PS: I tried both VS Copilot and Claude Desktop. Claude Desktop works but VS Copilot does not due to the nested structure of batch. Will look into it more.
Update on Batch
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
13-13: TheAutoRegister = falseflag is still not respected by the discovery logic.This was flagged in a previous review. Ensure the
CommandRegistry.AutoDiscoverCommandslogic checks theAutoRegisterproperty before registering tools, or remove this attribute if auto-registration is intended.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/BatchExecute.cs (2)
36-43: Consider documenting whymaxParallelismis parsed but unused.The
maxParallelismparameter is extracted and echoed in the response but has no functional effect since parallel execution isn't supported. A brief comment clarifying this is intentional (e.g., for API parity with the server-side batch_execute or future use) would improve maintainability.+ // Note: maxParallelism is captured for API parity with the server-side tool + // but has no effect since Unity commands run sequentially on the main thread. int? maxParallel = @params.Value<int?>("maxParallelism");
86-111: Consider adding protection against recursive batch execution.There's no check preventing
batch_executefrom being invoked within a batch, which could lead to deeply nested execution or resource exhaustion. While this requires deliberate misuse, adding a recursion guard or explicitly rejectingbatch_executeas a nested tool would make the implementation more robust.+ if (string.Equals(toolName, "batch_execute", StringComparison.OrdinalIgnoreCase)) + { + failureCount++; + commandResults.Add(new + { + success = false, + tool = toolName, + error = "Recursive batch_execute calls are not permitted." + }); + if (failFast) break; + continue; + } + try { var result = await CommandRegistry.InvokeCommandAsync(toolName, commandParams).ConfigureAwait(true);MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
267-316: Consider simplifying the tab mutual-exclusion logic.The current implementation works correctly but has somewhat complex callback logic to ensure at least one tab is always selected. Consider a simplified approach using radio button behavior or a single callback that switches to the other tab when one is deselected.
Additionally, lines 272-273 remove the "hidden" class from panels, but
SwitchPanel()immediately setsdisplaystyles. Verify if this class removal is necessary.Example simplification:
private void SetupTabs() { settingsTabToggle = rootVisualElement.Q<ToolbarToggle>("settings-tab"); toolsTabToggle = rootVisualElement.Q<ToolbarToggle>("tools-tab"); - settingsPanel?.RemoveFromClassList("hidden"); - toolsPanel?.RemoveFromClassList("hidden"); - if (settingsTabToggle != null) { settingsTabToggle.RegisterValueChangedCallback(evt => { - if (!evt.newValue) - { - if (toolsTabToggle != null && !toolsTabToggle.value) - { - settingsTabToggle.SetValueWithoutNotify(true); - } - return; - } - - SwitchPanel(ActivePanel.Settings); + if (evt.newValue) + { + SwitchPanel(ActivePanel.Settings); + } + else if (toolsTabToggle != null && !toolsTabToggle.value) + { + settingsTabToggle.SetValueWithoutNotify(true); + } }); } if (toolsTabToggle != null) { toolsTabToggle.RegisterValueChangedCallback(evt => { - if (!evt.newValue) - { - if (settingsTabToggle != null && !settingsTabToggle.value) - { - toolsTabToggle.SetValueWithoutNotify(true); - } - return; - } - - SwitchPanel(ActivePanel.Tools); + if (evt.newValue) + { + SwitchPanel(ActivePanel.Tools); + } + else if (settingsTabToggle != null && !settingsTabToggle.value) + { + toolsTabToggle.SetValueWithoutNotify(true); + } }); } var savedPanel = EditorPrefs.GetString(EditorPrefKeys.EditorWindowActivePanel, ActivePanel.Settings.ToString()); if (!Enum.TryParse(savedPanel, out ActivePanel initialPanel)) { initialPanel = ActivePanel.Settings; } SwitchPanel(initialPanel); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/BatchExecute.cs(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/Windows/MCPForUnityEditorWindow.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/MCPForUnityEditorWindow.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/MCPForUnityEditorWindow.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/BatchExecute.cs (1)
MCPForUnity/Editor/Tools/CommandRegistry.cs (2)
Task(257-279)CommandRegistry(37-437)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (3)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.cs (1)
System(39-39)MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (3)
McpToolsSection(16-269)McpToolsSection(29-34)Refresh(77-117)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-43)
🔇 Additional comments (10)
MCPForUnity/Editor/Tools/BatchExecute.cs (2)
16-34: LGTM!Input validation is comprehensive—checking for null params, empty commands array, and enforcing a reasonable batch size limit. Error messages are clear and actionable.
114-128: LGTM!Response aggregation is well-structured with clear success/failure delineation, appropriate use of
SuccessResponse/ErrorResponsewrappers, and useful metadata for consumers to understand batch execution outcomes.MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (8)
30-35: LGTM!The new fields for the Tools section and tab UI elements are appropriately declared and follow the existing field pattern in the class.
42-46: LGTM!The
ActivePanelenum provides clear state management for the tabbed interface with appropriate visibility scope.
131-152: LGTM!The panel and container queries include appropriate null checks with clear error logging, ensuring the UI elements are present before proceeding.
154-154: LGTM!The
SetupTabs()call is appropriately placed after panel and container validation, ensuring all required UI elements are present before configuring tab behavior.
163-191: LGTM!The refactoring to add section roots to dedicated containers (
settingsContainer) rather than directly to panels provides better UI structure and separation of concerns.
195-209: LGTM!The Tools section loading follows the established pattern for other sections and includes appropriate error handling. The
Refresh()call ensures tools are discovered and displayed immediately upon initialization.
318-336: LGTM!The
SwitchPanelmethod cleanly manages panel visibility and toggle states usingSetValueWithoutNotifyto avoid callback loops, and properly persists the selection via EditorPrefs for a consistent user experience across sessions.
346-366: LGTM!The asynchronous health check scheduling properly uses
EditorApplication.delayCallwith appropriate null checks and error handling to prevent crashes during cleanup or domain reloads.
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using MCPForUnity.Editor.Helpers; | ||
| using MCPForUnity.Editor.Services; | ||
| using MCPForUnity.Editor.Windows.Components.ClientConfig; | ||
| using MCPForUnity.Editor.Windows.Components.Connection; | ||
| using MCPForUnity.Editor.Windows.Components.Settings; | ||
| using UnityEditor; | ||
| using UnityEditor.UIElements; | ||
| using UnityEngine; | ||
| using UnityEngine.UIElements; | ||
| using MCPForUnity.Editor.Constants; | ||
| using MCPForUnity.Editor.Helpers; | ||
| using MCPForUnity.Editor.Services; | ||
| using MCPForUnity.Editor.Windows.Components.Settings; | ||
| using MCPForUnity.Editor.Windows.Components.Connection; | ||
| using MCPForUnity.Editor.Windows.Components.ClientConfig; | ||
| using MCPForUnity.Editor.Windows.Components.Tools; |
There was a problem hiding this comment.
Remove duplicate using statements.
Lines 14-19 duplicate the using statements from lines 5-9. Only the new imports (line 3 System.Linq, line 11 UnityEditor.UIElements, line 14 MCPForUnity.Editor.Constants, and line 20 MCPForUnity.Editor.Windows.Components.Tools) should be added.
Apply this diff to remove the duplicates:
using System;
using System.Collections.Generic;
+using System.Linq;
using System.Threading.Tasks;
using MCPForUnity.Editor.Helpers;
using MCPForUnity.Editor.Services;
using MCPForUnity.Editor.Windows.Components.ClientConfig;
using MCPForUnity.Editor.Windows.Components.Connection;
using MCPForUnity.Editor.Windows.Components.Settings;
using UnityEditor;
+using UnityEditor.UIElements;
using UnityEngine;
using UnityEngine.UIElements;
-using MCPForUnity.Editor.Constants;
-using MCPForUnity.Editor.Helpers;
-using MCPForUnity.Editor.Services;
-using MCPForUnity.Editor.Windows.Components.Settings;
-using MCPForUnity.Editor.Windows.Components.Connection;
-using MCPForUnity.Editor.Windows.Components.ClientConfig;
+using MCPForUnity.Editor.Constants;
using MCPForUnity.Editor.Windows.Components.Tools;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs around lines 3 to 20,
there are duplicate using statements (lines 14–19 repeat lines 5–9); remove the
duplicated imports so the file contains only the unique using lines: keep
System.Linq (line 3), UnityEditor.UIElements (line 11),
MCPForUnity.Editor.Constants (line 14), and
MCPForUnity.Editor.Windows.Components.Tools (line 20), and delete the repeated
MCPForUnity.Editor.Helpers, MCPForUnity.Editor.Services,
MCPForUnity.Editor.Windows.Components.Settings,
MCPForUnity.Editor.Windows.Components.Connection, and
MCPForUnity.Editor.Windows.Components.ClientConfig duplicates.
| internal static void CloseAllWindows() | ||
| { | ||
| var windows = OpenWindows.Where(window => window != null).ToArray(); | ||
| foreach (var window in windows) | ||
| { | ||
| window.Close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate with existing CloseAllOpenWindows method.
This new CloseAllWindows method duplicates the functionality of the existing CloseAllOpenWindows method (lines 69-88), which has more robust error handling with try-catch blocks and logging. Consider removing this method and using CloseAllOpenWindows instead, or consolidate both implementations.
If you need to keep both methods for different access levels (internal vs public), apply this diff to align error handling:
internal static void CloseAllWindows()
{
- var windows = OpenWindows.Where(window => window != null).ToArray();
+ if (OpenWindows.Count == 0)
+ return;
+
+ var windows = new MCPForUnityEditorWindow[OpenWindows.Count];
+ OpenWindows.CopyTo(windows);
foreach (var window in windows)
{
- window.Close();
+ try
+ {
+ window?.Close();
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"Error closing MCP window: {ex.Message}");
+ }
}
}🤖 Prompt for AI Agents
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs lines 48-55: The
CloseAllWindows method duplicates CloseAllOpenWindows (lines 69-88) and should
be consolidated; either remove CloseAllWindows and update callers to use
CloseAllOpenWindows, or make CloseAllWindows delegate to CloseAllOpenWindows
(preserving its internal accessibility) and adopt the same try-catch and logging
from CloseAllOpenWindows so error handling is identical. Ensure no callers are
left referencing the removed method (update references), and if keeping both,
forward from CloseAllWindows to the existing implementation rather than
duplicating logic.
1 participant
2.5 Some details: It right now detects the tools under Packages/MCPForUnity/Editor/Tools as built-in, and other tools as custom tools. I am still wondering how to untoggle it, looking for helps if possible. (It is a good-to-have feature for custom tools at least)
Here is the current visualization of the tool panel (everything in settings panel remains the same):
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.