Feat/robust component resolver #256
Conversation
- Replace Assembly.LoadFrom with already-loaded assembly search - Prioritize Player assemblies over Editor assemblies using CompilationPipeline - Support both short names and fully-qualified component names - Add comprehensive caching and error handling with disambiguation - Use Undo.AddComponent for proper editor integration - Handle ReflectionTypeLoadException safely during type enumeration - Add fallback to TypeCache for comprehensive type discovery in Editor Fixes component resolution across custom assembly definitions and eliminates "Could not load the file 'Assembly-CSharp-Editor'" errors. Tested with: - Built-in Unity components (Rigidbody, MeshRenderer, AudioSource) - Custom user scripts in Assembly-CSharp (TicTacToe3D) - Custom assembly definition components (TestNamespace.CustomComponent) - Error handling for non-existent components 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add intelligent property name suggestions when property setting fails - Implement GetAllComponentProperties to enumerate available properties - Add rule-based AI algorithm for property name matching (camelCase, spaces, etc.) - Include comprehensive error messages with suggestions and full property lists - Add Levenshtein distance calculation for fuzzy string matching - Cache suggestions to improve performance on repeated queries - Add comprehensive unit tests (11 tests) covering all ComponentResolver scenarios - Add InternalsVisibleTo attribute for test access to internal classes Examples of improved error messages: - "Max Reach Distance" → "Did you mean: maxReachDistance?" - Shows all available properties when property not found - Handles Unity Inspector display names vs actual field names All tests passing (21/21) including new ComponentResolver test suite. The system eliminates silent property setting failures and provides actionable feedback to developers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ent property matching - Add AIPropertyMatchingTests.cs with 14 tests covering property enumeration, fuzzy matching, caching, and Unity naming conventions - Add ManageGameObjectTests.cs with 10 integration tests for component resolution, property matching, and error handling - Add ComponentResolverTests.cs.meta for existing comprehensive ComponentResolver tests - Add AssemblyInfo.cs.meta for test assembly access - Fix ManageGameObject.HandleCommand null parameter handling to prevent NullReferenceException All 45 unit tests now pass, providing full coverage of: - Robust component resolution avoiding Assembly.LoadFrom - Intelligent property name suggestions using rule-based fuzzy matching - Assembly definition (asmdef) support via CompilationPipeline - Comprehensive error handling with helpful suggestions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a TestAsmdef assembly and CustomComponent MonoBehaviour; introduces extensive EditMode tests for component resolution and AI-style property suggestions; exposes editor internals to tests; centralizes type lookup into a new ComponentResolver and integrates it across ManageGameObject and ManageAsset with nested property setting and richer error/suggestion flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant MG as ManageGameObject
participant MA as ManageAsset
participant CR as ComponentResolver
participant Ser as Helpers.GameObjectSerializer
Caller->>MG: HandleCommand(params)
alt params == null
MG-->>Caller: Error (null params)
else target endsWith ".prefab" and action in {modify, set_component_property}
note over MG,MA #e6f7ff: Prefab redirected to ManageAsset
MG->>MA: Redirected params
MA-->>Caller: Result / Error
else
MG->>CR: TryResolve(componentName)
alt resolved
MG->>MG: Get/Set component, SetNestedProperty
MG->>Ser: Convert/serialize inputs
Ser-->>MG: Converted values
MG-->>Caller: Success
else unresolved
MG->>CR: GetAIPropertySuggestions(input, availableProps)
MG-->>Caller: Error + suggestions
end
end
sequenceDiagram
autonumber
actor Tool
participant MA as ManageAsset
participant CR as ComponentResolver
Tool->>MA: Create ScriptableObject(scriptClassName)
MA->>CR: TryResolve(scriptClassName)
alt resolved
MA-->>Tool: Create asset with resolved Type
else
MA-->>Tool: Error: Script class 'X' invalid: <reason>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (3)
204-213: Surface resolver error context when ScriptableObject type resolution fails.If resolution fails, include
errorfrom ComponentResolver in the user-facing message to aid debugging.- Type scriptType = ComponentResolver.TryResolve(scriptClassName, out var resolvedType, out var error) ? resolvedType : null; + Type scriptType = ComponentResolver.TryResolve(scriptClassName, out var resolvedType, out var error) ? resolvedType : null; if ( scriptType == null || !typeof(ScriptableObject).IsAssignableFrom(scriptType) ) { - return Response.Error( - $"Script class '{scriptClassName}' not found or does not inherit from ScriptableObject." - ); + var reason = scriptType == null + ? (string.IsNullOrEmpty(error) ? "Type not found." : error) + : "Type found but does not inherit from ScriptableObject."; + return Response.Error($"Script class '{scriptClassName}' invalid: {reason}"); }
356-366: Use ComponentResolver for component lookup instead of GetComponent(string).
GetComponent(string)is fragile and may not behave as intended in C#. Resolve the type first (handles asmdefs/player assemblies/ambiguity), then callGetComponent(Type).- // Using GetComponent(string) is convenient but might require exact type name or be ambiguous. - // Consider using ComponentResolver if needed for more complex scenarios. - Component targetComponent = gameObject.GetComponent(componentName); + // Resolve component type via ComponentResolver, then fetch by Type + Component targetComponent = null; + if (ComponentResolver.TryResolve(componentName, out var compType, out var compError)) + { + targetComponent = gameObject.GetComponent(compType); + } + else + { + Debug.LogWarning( + $"[ManageAsset.ModifyAsset] Failed to resolve component '{componentName}' on '{gameObject.name}': {compError}" + ); + }
936-947: Fix operator precedence bug that can pass with null propName and cause exceptions.Parentheses are required; otherwise the condition can be true when
propNameis null but value is integer.- if ( - !string.IsNullOrEmpty(propName) && floatProps["value"]?.Type == JTokenType.Float - || floatProps["value"]?.Type == JTokenType.Integer - ) + if ( + !string.IsNullOrEmpty(propName) && + (floatProps["value"]?.Type == JTokenType.Float || floatProps["value"]?.Type == JTokenType.Integer) + )UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
287-297: Primitive creation leaks a GameObject on early error.If name is missing, the code returns after creating the primitive without cleaning it up.
Apply:
- // Set name *after* creation for primitives - if (!string.IsNullOrEmpty(name)) - newGo.name = name; - else - return Response.Error( - "'name' parameter is required when creating a primitive." - ); // Name is essential + // Set name *after* creation for primitives + if (!string.IsNullOrEmpty(name)) + { + newGo.name = name; + } + else + { + UnityEngine.Object.DestroyImmediate(newGo); // cleanup leak + return Response.Error( + "'name' parameter is required when creating a primitive." + ); + }
1496-1503: Resolve component type before GetComponent to avoid string-based lookup pitfalls.Using GetComponent(string) ignores namespaces and risks ambiguity. Resolve to Type and call GetComponent(Type), with string fallback.
- Component targetComponent = targetComponentInstance ?? targetGo.GetComponent(compName); + Component targetComponent = targetComponentInstance; + if (targetComponent == null) + { + var compType = FindType(compName); + targetComponent = compType != null + ? targetGo.GetComponent(compType) + : targetGo.GetComponent(compName); // fallback + }
1600-1631: Allow setting private [SerializeField] fields when present.You surface serialized private fields in suggestions but SetProperty cannot set them. Include NonPublic field lookup gated by [SerializeField].
- else - { - FieldInfo fieldInfo = type.GetField(memberName, flags); + else + { + FieldInfo fieldInfo = type.GetField(memberName, flags); if (fieldInfo != null) // Check if !IsLiteral? { // Use the inputSerializer for conversion object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer); if (convertedValue != null || value.Type == JTokenType.Null) // Allow setting null { fieldInfo.SetValue(target, convertedValue); return true; } else { Debug.LogWarning($"[SetProperty] Conversion failed for field '{memberName}' (Type: {fieldInfo.FieldType.Name}) from token: {value.ToString(Formatting.None)}"); } } + else + { + // Try NonPublic [SerializeField] fields + var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase); + if (npField != null && npField.GetCustomAttribute<SerializeField>() != null) + { + object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) + { + npField.SetValue(target, convertedValue); + return true; + } + } + } }
🧹 Nitpick comments (22)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (5)
361-369: Record Undo before mutating components to align with Editor best practices.Adds proper Undo support when modifying prefab/component properties.
- modified |= ApplyObjectProperties( + // Enable Undo of component changes + Undo.RecordObject(targetComponent, $"Modify {componentName} on {gameObject.name}"); + modified |= ApplyObjectProperties( targetComponent, componentProperties );
396-405: Also record Undo for Material/ScriptableObject/Importer edits.Improves UX parity across asset types.
- // Apply properties directly to the material. If this modifies, it sets modified=true. + // Apply properties directly to the material. If this modifies, it sets modified=true. + Undo.RecordObject(material, $"Modify Material {material.name}"); // Use |= in case the asset was already marked modified by previous logic (though unlikely here) modified |= ApplyMaterialProperties(material, properties);And similarly when
asset is ScriptableObject soand before importer changes:- modified |= ApplyObjectProperties(so, properties); // General helper + Undo.RecordObject(so, $"Modify {so.name}"); + modified |= ApplyObjectProperties(so, properties); // General helper- bool importerModified = ApplyObjectProperties(textureImporter, properties); + Undo.RecordObject(textureImporter, $"Modify TextureImporter {Path.GetFileName(fullPath)}"); + bool importerModified = ApplyObjectProperties(textureImporter, properties);
1072-1081: Offer AI-backed property name suggestions when a set fails.Leverage the new suggestion utilities to guide users if a property isn’t found.
- if (SetPropertyOrField(target, propName, propValue, type)) - { - modified = true; - } + if (SetPropertyOrField(target, propName, propValue, type)) + { + modified = true; + } + else if (target is Component comp) + { + var available = ComponentResolver.GetAllComponentProperties(type); + var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, available); + if (suggestions.Count > 0) + { + Debug.LogWarning($"[ApplyObjectProperties] '{propName}' not found on {type.Name}. Did you mean: {string.Join(", ", suggestions)}?"); + } + }
1024-1057: Simplify (and harden) combine parsing with Enum.TryParse and case-insensitivity.Reduces duplication, handles mixed case, and only marks modified when value actually changes.
- List<String> averageList = new List<String> { "ave", "Ave", "average", "Average" }; - List<String> multiplyList = new List<String> { "mul", "Mul", "mult", "Mult", "multiply", "Multiply" }; - List<String> minimumList = new List<String> { "min", "Min", "minimum", "Minimum" }; - List<String> maximumList = new List<String> { "max", "Max", "maximum", "Maximum" }; + // Accept common shorthands via a small alias map + var combineAliases = new Dictionary<string, PhysicsMaterialCombine>(StringComparer.OrdinalIgnoreCase) + { + ["ave"] = PhysicsMaterialCombine.Average, + ["average"] = PhysicsMaterialCombine.Average, + ["mul"] = PhysicsMaterialCombine.Multiply, + ["mult"] = PhysicsMaterialCombine.Multiply, + ["multiply"] = PhysicsMaterialCombine.Multiply, + ["min"] = PhysicsMaterialCombine.Minimum, + ["minimum"] = PhysicsMaterialCombine.Minimum, + ["max"] = PhysicsMaterialCombine.Maximum, + ["maximum"] = PhysicsMaterialCombine.Maximum + }; @@ - if (properties["frictionCombine"]?.Type == JTokenType.String) + if (properties["frictionCombine"]?.Type == JTokenType.String) { - string frictionCombine = properties["frictionCombine"].ToString(); - if (averageList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Average; - else if (multiplyList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Multiply; - else if (minimumList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Minimum; - else if (maximumList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Maximum; - modified = true; + string raw = properties["frictionCombine"].ToString(); + PhysicsMaterialCombine parsed; + if (combineAliases.TryGetValue(raw, out parsed) || + Enum.TryParse(raw, true, out parsed)) + { + if (pmat.frictionCombine != parsed) + { + pmat.frictionCombine = parsed; + modified = true; + } + } } @@ - if (properties["bounceCombine"]?.Type == JTokenType.String) + if (properties["bounceCombine"]?.Type == JTokenType.String) { - string bounceCombine = properties["bounceCombine"].ToString(); - if (averageList.Contains(bounceCombine)) - pmat.bounceCombine = PhysicsMaterialCombine.Average; - ... - modified = true; + string raw = properties["bounceCombine"].ToString(); + PhysicsMaterialCombine parsed; + if (combineAliases.TryGetValue(raw, out parsed) || + Enum.TryParse(raw, true, out parsed)) + { + if (pmat.bounceCombine != parsed) + { + pmat.bounceCombine = parsed; + modified = true; + } + } }
809-819: Normalize to canonical 'Assets/' casing (nit).Unity APIs accept lowercase, but canonical casing avoids surprises on case-sensitive filesystems.
- if (!path.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) + if (!path.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) { - return "Assets/" + path.TrimStart('/'); + var tail = path.StartsWith("assets/", StringComparison.OrdinalIgnoreCase) + ? path.Substring(6).TrimStart('/', '\\') + : path.TrimStart('/', '\\'); + return "Assets/" + tail; }TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs (1)
5-17: LGTM; simple test component is well-scoped.Optional: add
[AddComponentMenu("Test/CustomComponent")]to make it easier to add from the editor during manual tests.UnityMcpBridge/Editor/Tools/ManageGameObject.cs (8)
49-51: Confirm default for includeNonPublicSerialized.Defaulting to true exposes private [SerializeField] data in editor responses. Confirm this is intentional; otherwise default to false and let callers opt in.
878-949: Avoid O(n^2) list reordering in get_components.Iterating backward and inserting at index 0 causes repeated shifts. You already copied the array; iterate forward and append.
- var componentData = new List<object>(); - - for (int i = componentCount - 1; i >= 0; i--) // Iterate backwards over the COPY - { - Component c = componentsToIterate[i]; // Use the copy + var componentData = new List<object>(componentCount); + for (int i = 0; i < componentCount; i++) + { + Component c = componentsToIterate[i]; if (c == null) { continue; // Safety check } try { var data = Helpers.GameObjectSerializer.GetComponentData(c, includeNonPublicSerialized); if (data != null) // Ensure GetComponentData didn't return null { - componentData.Insert(0, data); // Insert at beginning to maintain original order in final list + componentData.Add(data); } } catch (Exception ex) { Debug.LogError($"[GetComponentsFromTarget REVERSE for] Error processing component {c.GetType().FullName} (ID: {c.GetInstanceID()}) on {targetGo.name}: {ex.Message}\n{ex.StackTrace}"); - componentData.Insert(0, new JObject( // Insert error marker at beginning + componentData.Add(new JObject( new JProperty("typeName", c.GetType().FullName + " (Serialization Error)"), new JProperty("instanceID", c.GetInstanceID()), new JProperty("error", ex.Message) )); } } - // Debug.Log($"[GetComponentsFromTarget] Finished REVERSE for loop."); + // Debug.Log($"[GetComponentsFromTarget] Finished forward loop.");
1106-1127: Deduplicate ParseVector3; reuse Vector3Helper for consistency.You already have Helpers.Vector3Helper.ParseVector3(JArray). Wrap it to return nullable instead of reimplementing.
- private static Vector3? ParseVector3(JArray array) - { - if (array != null && array.Count == 3) - { - try - { - return new Vector3( - array[0].ToObject<float>(), - array[1].ToObject<float>(), - array[2].ToObject<float>() - ); - } - catch (Exception ex) - { - Debug.LogWarning($"Failed to parse JArray as Vector3: {array}. Error: {ex.Message}"); - } - } - return null; - } + private static Vector3? ParseVector3(JArray array) + { + if (array == null) return null; + try { return MCPForUnity.Editor.Helpers.Vector3Helper.ParseVector3(array); } + catch (Exception ex) + { + Debug.LogWarning($"Failed to parse Vector3: {ex.Message}"); + return null; + } + }
1570-1589: Avoid per-call JsonSerializer allocations in SetProperty.Create a single static serializer with converters and reuse it.
- JsonSerializerSettings inputSerializerSettings = new JsonSerializerSettings - { - Converters = new List<JsonConverter> - { - // Add specific converters needed for INPUT deserialization if different from output - new Vector3Converter(), - new Vector2Converter(), - new QuaternionConverter(), - new ColorConverter(), - new RectConverter(), - new BoundsConverter(), - new UnityEngineObjectConverter() // Crucial for finding references from instructions - } - // No ReferenceLoopHandling needed typically for input - }; - JsonSerializer inputSerializer = JsonSerializer.Create(inputSerializerSettings); + var inputSerializer = InputSerializer; // reuse static instanceAdd (outside this hunk, at class scope):
// Serializer reused across setters private static readonly JsonSerializer InputSerializer = JsonSerializer.Create( new JsonSerializerSettings { Converters = new List<JsonConverter> { new Vector3Converter(), new Vector2Converter(), new QuaternionConverter(), new ColorConverter(), new RectConverter(), new BoundsConverter(), new UnityEngineObjectConverter() } });
840-845: Avoid reflection to build delete confirmation message.Store the name in the loop and reuse it instead of reflecting anonymous types.
- string message = - targets.Count == 1 - ? $"GameObject '{deletedObjects[0].GetType().GetProperty("name").GetValue(deletedObjects[0])}' deleted successfully." + string message = + targets.Count == 1 + ? $"GameObject '{((dynamic)deletedObjects[0]).name}' deleted successfully." : $"{deletedObjects.Count} GameObjects deleted successfully.";Or keep a parallel List deletedNames to compose the message.
2161-2177: Use short-name cache to avoid repeated scans.You cache by Name but never read it. Check CacheByName for queries without a dot.
- public static bool TryResolve(string nameOrFullName, out Type type, out string error) + public static bool TryResolve(string nameOrFullName, out Type type, out string error) { error = string.Empty; type = null!; // Handle null/empty input if (string.IsNullOrWhiteSpace(nameOrFullName)) { error = "Component name cannot be null or empty"; return false; } - // 1) Exact FQN via Type.GetType + // 1) Cache hit + if (!nameOrFullName.Contains('.') && CacheByName.TryGetValue(nameOrFullName, out type)) return true; + if (CacheByFqn.TryGetValue(nameOrFullName, out type)) return true; + // 2) Exact FQN via Type.GetType - if (CacheByFqn.TryGetValue(nameOrFullName, out type)) return true; type = Type.GetType(nameOrFullName, throwOnError: false); if (IsValidComponent(type)) { Cache(type); return true; } - // 2) Search loaded assemblies (prefer Player assemblies) + // 3) Search loaded assemblies (prefer Player assemblies)
2304-2320: Remove unused 'prompt' variable.Dead code; keep noise down.
- var prompt = $"A Unity developer is trying to set a component property but used an incorrect name.\n\n" + - $"User requested: \"{userInput}\"\n" + - $"Available properties: [{string.Join(", ", availableProperties)}]\n\n" + - $"Find 1-3 most likely matches considering:\n" + - $"- Unity Inspector display names vs actual field names (e.g., \"Max Reach Distance\" → \"maxReachDistance\")\n" + - $"- camelCase vs PascalCase vs spaces\n" + - $"- Similar meaning/semantics\n" + - $"- Common Unity naming patterns\n\n" + - $"Return ONLY the matching property names, comma-separated, no quotes or explanation.\n" + - $"If confidence is low (<70%), return empty string.\n\n" + - $"Examples:\n" + - $"- \"Max Reach Distance\" → \"maxReachDistance\"\n" + - $"- \"Health Points\" → \"healthPoints, hp\"\n" + - $"- \"Move Speed\" → \"moveSpeed, movementSpeed\""; + // Placeholder kept for future AI integration (removed unused variable).
2409-2428: Drop duplicate ParseVector3 in ComponentResolver region.This method isn’t used by the resolver and duplicates logic elsewhere. Remove to reduce maintenance.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs (1)
5-5: Remove unused using static.The static import of ManageGameObject isn’t needed here.
-using static MCPForUnity.Editor.Tools.ManageGameObject;TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
33-39: Strengthen assertion by inspecting Response.Success flag.Parse the result to assert error vs success explicitly.
- Assert.IsNotNull(result, "Should return a result object"); - // Note: Actual error checking would need access to Response structure + Assert.IsNotNull(result); + dynamic r = result; + Assert.IsFalse((bool)r.success, "Expected error response for null params");
50-69: Assert creation success via response and cleanup.Validate Response.success/message to catch regressions.
- Assert.IsNotNull(result, "Should return a result for valid create action"); + Assert.IsNotNull(result); + dynamic r = result; + Assert.IsTrue((bool)r.success, "Expected success for create action");TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (5)
94-100: Use an actual typo to exercise Levenshtein, not just case normalization.Current input ("movespeed") only tests case-insensitivity. Make it a real typo so we validate edit-distance logic.
- public void GetAIPropertySuggestions_FindsSimilarStrings_WithTypos() + public void GetAIPropertySuggestions_HandlesTypos_ViaLevenshtein() @@ - var suggestions = ComponentResolver.GetAIPropertySuggestions("movespeed", sampleProperties); // missing capital S + var suggestions = ComponentResolver.GetAIPropertySuggestions("moev speed", sampleProperties); // transposed letters + space @@ - Assert.Contains("moveSpeed", suggestions, "Should find moveSpeed despite missing capital"); + Assert.Contains("moveSpeed", suggestions, "Should find moveSpeed despite typo");
102-109: Clarify expectations for “semantic” matches.As written, this always passes (non-null list). Either assert the current behavior explicitly (empty until synonyms are added) or mark with a TODO referencing planned semantic mapping.
If you want to lock current behavior:
- // Note: Current algorithm might not find "mass" but should handle it gracefully - Assert.IsNotNull(suggestions, "Should return valid suggestions list"); + // Note: Rule-based matcher has no synonym map yet; expect no suggestions for "weight". + Assert.IsEmpty(suggestions, "Should return empty until semantic mapping is added");
59-66: Add whitespace-only input case.Covers IsNullOrWhiteSpace guard and prevents regressions.
public void GetAIPropertySuggestions_ReturnsEmpty_ForEmptyInput() { var suggestions = ComponentResolver.GetAIPropertySuggestions("", sampleProperties); Assert.IsEmpty(suggestions, "Empty input should return no suggestions"); } + + [Test] + public void GetAIPropertySuggestions_ReturnsEmpty_ForWhitespaceInput() + { + var suggestions = ComponentResolver.GetAIPropertySuggestions(" ", sampleProperties); + Assert.IsEmpty(suggestions, "Whitespace input should return no suggestions"); + }
135-147: Include hyphen/underscore input variants for Unity-style names.Strengthens delimiter normalization coverage without changing behavior.
var suggestions1 = ComponentResolver.GetAIPropertySuggestions("is kinematic", unityStyleProperties); var suggestions2 = ComponentResolver.GetAIPropertySuggestions("use gravity", unityStyleProperties); var suggestions3 = ComponentResolver.GetAIPropertySuggestions("max linear velocity", unityStyleProperties); + var suggestions4 = ComponentResolver.GetAIPropertySuggestions("max-linear-velocity", unityStyleProperties); + var suggestions5 = ComponentResolver.GetAIPropertySuggestions("max_linear_velocity", unityStyleProperties); Assert.Contains("isKinematic", suggestions1, "Should handle 'is' prefix convention"); Assert.Contains("useGravity", suggestions2, "Should handle 'use' prefix convention"); Assert.Contains("maxLinearVelocity", suggestions3, "Should handle 'max' prefix convention"); + Assert.Contains("maxLinearVelocity", suggestions4, "Should handle hyphen-delimited input"); + Assert.Contains("maxLinearVelocity", suggestions5, "Should handle underscore-delimited input");
120-133: Verify result stability across different property orders.Caching key is order-sensitive, but result ordering should be distance-driven and independent of list order. Add a test to lock this in.
public void GetAIPropertySuggestions_CachesResults() { var input = "Max Reach Distance"; // First call var suggestions1 = ComponentResolver.GetAIPropertySuggestions(input, sampleProperties); // Second call should use cache (tested indirectly by ensuring consistency) var suggestions2 = ComponentResolver.GetAIPropertySuggestions(input, sampleProperties); Assert.AreEqual(suggestions1.Count, suggestions2.Count, "Cached results should be consistent"); CollectionAssert.AreEqual(suggestions1, suggestions2, "Cached results should be identical"); } + + [Test] + public void GetAIPropertySuggestions_ResultsIndependentOfPropertyOrder() + { + var input = "max distance"; + var reversed = new List<string>(sampleProperties); + reversed.Reverse(); + var s1 = ComponentResolver.GetAIPropertySuggestions(input, sampleProperties); + var s2 = ComponentResolver.GetAIPropertySuggestions(input, reversed); + CollectionAssert.AreEqual(s1, s2, "Results should not depend on availableProperties order"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs.meta(1 hunks)UnityMcpBridge/Editor/AssemblyInfo.cs(1 hunks)UnityMcpBridge/Editor/AssemblyInfo.cs.meta(1 hunks)UnityMcpBridge/Editor/Tools/ManageAsset.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
UnityEngine(2026-2123)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (12)
UnityEngine(2026-2123)GameObject(1132-1155)ManageGameObject(22-2145)HandleCommand(26-159)ComponentResolver(2151-2432)TryResolve(2161-2196)Type(2130-2144)List(1160-1319)List(2211-2248)List(2271-2289)List(2294-2333)List(2341-2376)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
Type(2130-2144)ComponentResolver(2151-2432)TryResolve(2161-2196)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (7)
ManageGameObject(22-2145)List(1160-1319)List(2211-2248)List(2271-2289)List(2294-2333)List(2341-2376)ComponentResolver(2151-2432)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
System(2267-2333)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)UnityMcpBridge/Editor/Helpers/Vector3Helper.cs (1)
Vector3(17-22)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (5)
UnityEngine(2026-2123)ManageGameObject(22-2145)ComponentResolver(2151-2432)TryResolve(2161-2196)Type(2130-2144)
🔇 Additional comments (16)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs.meta (2)
1-2: Meta file present with GUID — OK.Nothing to change.
1-2: No duplicate Unity GUIDs found. Sanity check across all .meta files confirms each GUID is unique.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta (1)
1-2: Meta file looks good.GUID recorded; no issues.
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs.meta (1)
1-2: Meta file OK for CustomComponent.All good.
UnityMcpBridge/Editor/AssemblyInfo.cs (2)
1-3: Expose internals to EditMode tests — good placement.Attribute is in Editor assembly; safe for player builds.
3-3: Verified asmdef name matches InternalsVisibleTo attribute.
TheMCPForUnityTests.Editor.asmdefdefines"name": "MCPForUnityTests.EditMode", matching the attribute.TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef (1)
1-14: Asmdef added — OK for test fixture assembly.Name/rootNamespace are consistent; defaults are fine.
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef.meta (1)
1-7: Meta looks correct and consistent with the new asmdef.GUID present, importer block valid. Nothing to change.
UnityMcpBridge/Editor/AssemblyInfo.cs.meta (1)
1-2: Meta addition is fine.GUID only; aligns with adding AssemblyInfo.cs for InternalsVisibleTo.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs.meta (1)
1-2: Meta file is fine.Nothing actionable.
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
28-31: Good guard for null parameters.Early-return with a clear error keeps the handler robust.
2132-2144: FindType wrapper correctly delegates and logs resolver diagnostics.This centralization is good.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
111-126: Good coverage of resolver against built-ins.Gracefully handling abstract types with error checks is sensible.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (3)
1-169: Solid coverage and clear intent.Good breadth: null/empty inputs, exact/word/typo matches, limits, caching, Unity naming. Tests read well and should be stable across Unity versions.
149-157: Exact-match prioritization test is on point.Good assertion that distance 0 ranks first.
159-167: Case-insensitivity covered well.Validates both upper and lower cases against the same target.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs
Outdated
Show resolved
Hide resolved
Critical Bug Fixes: - Fix operator precedence bug in ManageAsset.cs that could cause null reference exceptions - Fix GameObject memory leak in primitive creation when name validation fails - Add proper cleanup with DestroyImmediate when primitive creation fails ComponentResolver Integration: - Replace fragile string-based GetComponent() calls with robust ComponentResolver - Add ComponentResolver integration in ManageAsset.cs for component lookups - Add fallback to string-based lookup in ManageGameObject.cs for compatibility Enhanced Error Handling: - Surface specific ComponentResolver error context in ScriptableObject creation failures - Add support for setting private [SerializeField] fields in property matching - Improve debugging with detailed error messages Assembly Definition Fixes: - Configure TestAsmdef as Editor-only to prevent build bloat - Add explicit TestAsmdef reference to test assembly for proper component resolution - Fix ComponentResolverTests to use accessible CustomComponent instead of TicTacToe3D Code Quality: - Disable nullable reference types for legacy codebase to eliminate 100+ warnings - Maintain backward compatibility while improving reliability All 45 unit tests pass, ensuring no regressions while significantly improving robustness. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. @dsarno Thank you for the correction! You're absolutely right about the The automated review comment incorrectly assumed ComponentResolver was a sibling type in the same namespace. Your clarification about the nested class architecture and the resulting compilation errors (CS0117) without the static import is spot on. ✏️ Learnings added
|
Performance & Quality Improvements: - Add shared JsonSerializer to eliminate per-call allocation overhead - Optimize ComponentResolver with CacheByName for short-name lookups - Deduplicate Vector3 parsing implementations to reduce maintenance burden - Improve property name normalization for better fuzzy matching quality - Reduce log noise by avoiding duplicate component resolution warnings Code Quality: - Keep using static import for ComponentResolver (CodeRabbit was incorrect about this) - Normalize property names consistently in AI suggestions algorithm - Remove duplicate ParseVector3 implementation TestAsmdef Fix: - Revert TestAsmdef back to runtime-compatible (remove "includePlatforms": ["Editor"]) - CustomComponent now works correctly for asmdef testing as originally intended - Validates that ComponentResolver properly handles both short and FQN for asmdef components Live Testing Validation: - All ComponentResolver functionality verified through live MCP connection - AI property matching working perfectly with natural language input - Assembly definition support fully functional for both default and custom assemblies - Error handling provides helpful suggestions and complete context All 45 C# tests + 31 Python tests still passing. Production ready! 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (2)
358-373: Gate duplicate warnings when resolution already failed.Currently, when resolution fails you log both the resolver failure and the “component not found” warning, creating noise. Skip the second warning if resolution didn’t succeed.
Apply:
- // Only warn about resolution failure if component also not found - if (targetComponent == null && !resolved) + // Only warn about resolution failure; avoid duplicate "not found" below + if (!resolved) { Debug.LogWarning( $"[ManageAsset.ModifyAsset] Failed to resolve component '{componentName}' on '{gameObject.name}': {compError}" ); } ... - else + else if (targetComponent == null && resolved) { // Log a warning if a specified component couldn't be found Debug.LogWarning( $"[ManageAsset.ModifyAsset] Component '{componentName}' not found on GameObject '{gameObject.name}' in asset '{fullPath}'. Skipping modification for this component." ); }Also applies to: 385-389
1037-1054: Normalize combine-mode parsing and avoid per-call allocations.Use a single case-insensitive parser or cached HashSet with OrdinalIgnoreCase. This trims allocations on every call and simplifies maintenance.
- List<String> averageList = new List<String> { "ave", "Ave", "average", "Average" }; - List<String> multiplyList = new List<String> { "mul", "Mul", "mult", "Mult", "multiply", "Multiply" }; - List<String> minimumList = new List<String> { "min", "Min", "minimum", "Minimum" }; - List<String> maximumList = new List<String> { "max", "Max", "maximum", "Maximum" }; + static PhysicsMaterialCombine ParseCombine(string s) + { + if (string.IsNullOrEmpty(s)) return PhysicsMaterialCombine.Average; + s = s.Trim().ToLowerInvariant(); + return s switch + { + "ave" or "average" => PhysicsMaterialCombine.Average, + "mul" or "mult" or "multiply" => PhysicsMaterialCombine.Multiply, + "min" or "minimum" => PhysicsMaterialCombine.Minimum, + "max" or "maximum" => PhysicsMaterialCombine.Maximum, + _ => PhysicsMaterialCombine.Average + }; + } ... - string frictionCombine = properties["frictionCombine"].ToString(); - if (averageList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Average; - else if (multiplyList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Multiply; - else if (minimumList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Minimum; - else if (maximumList.Contains(frictionCombine)) - pmat.frictionCombine = PhysicsMaterialCombine.Maximum; + pmat.frictionCombine = ParseCombine(properties["frictionCombine"].ToString()); modified = true; ... - string bounceCombine = properties["bounceCombine"].ToString(); - if (averageList.Contains(bounceCombine)) - pmat.bounceCombine = PhysicsMaterialCombine.Average; - else if (multiplyList.Contains(bounceCombine)) - pmat.bounceCombine = PhysicsMaterialCombine.Multiply; - else if (minimumList.Contains(bounceCombine)) - pmat.bounceCombine = PhysicsMaterialCombine.Minimum; - else if (maximumList.Contains(bounceCombine)) - pmat.bounceCombine = PhysicsMaterialCombine.Maximum; + pmat.bounceCombine = ParseCombine(properties["bounceCombine"].ToString()); modified = true;Also applies to: 1058-1070
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (6)
1-1: Avoid file-wide nullable suppression.Prefer enabling nullable and adding targeted suppressions or annotations where needed. This keeps nullability checks useful across the file.
-#nullable disable +#nullable enableIf migration right now is heavy, limit disable to the few regions that need it with pragmas.
1114-1147: Deduplicate Vector3 parsing via a shared helper.We already have Helpers/Vector3Helper.ParseVector3; consider centralizing to one helper (e.g., add Vector3Helper.TryParse(JArray, out Vector3) that doesn’t throw) and call it here to reduce divergence.
- private static Vector3? ParseVector3(JArray array) - { - if (array != null && array.Count == 3) - { - try - { - return new Vector3( - array[0].ToObject<float>(), - array[1].ToObject<float>(), - array[2].ToObject<float>() - ); - } - catch (Exception ex) - { - Debug.LogWarning($"Failed to parse JArray as Vector3: {array}. Error: {ex.Message}"); - } - } - return null; - } + private static Vector3? ParseVector3(JArray array) + { + return Helpers.Vector3Helper.TryParse(array, out var v) ? v : (Vector3?)null; + }Additional helper (in Helpers/Vector3Helper.cs) to support this:
public static bool TryParse(JArray array, out Vector3 v) { v = default; if (array is { Count: 3 }) { try { v = new Vector3(array[0].ToObject<float>(), array[1].ToObject<float>(), array[2].ToObject<float>()); return true; } catch { } } return false; }
918-961: Avoid O(n^2) inserts while preserving order.Inserting at index 0 repeatedly shifts the list. Iterate forward and Add, or keep reverse build and call Reverse at the end.
- for (int i = componentCount - 1; i >= 0; i--) // Iterate backwards over the COPY + for (int i = 0; i < componentCount; i++) // Iterate forward { - Component c = componentsToIterate[i]; // Use the copy + Component c = componentsToIterate[i]; ... - if (data != null) // Ensure GetComponentData didn't return null - { - componentData.Insert(0, data); // Insert at beginning to maintain original order in final list - } + if (data != null) componentData.Add(data); ... - componentData.Insert(0, new JObject( + componentData.Add(new JObject(
1647-1656: NonPublic [SerializeField] lookup should be case-sensitive.Using IgnoreCase here risks writing the wrong private backing field. Keep it strict for NonPublic, while public members can remain IgnoreCase.
- var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase); + var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance);
1782-1854: Skip redundant writes to avoid unnecessary dirty flags.Optionally compare current and new values before setting nested fields/properties, mirroring your equality check patterns elsewhere. Helps reduce prefab/scene churn.
- if (convertedValue != null || value.Type == JTokenType.Null) + if (convertedValue != null || value.Type == JTokenType.Null) { - finalPropInfo.SetValue(currentObject, convertedValue); + if (!Equals(finalPropInfo.GetValue(currentObject), convertedValue)) + finalPropInfo.SetValue(currentObject, convertedValue); return true; } ... - if (convertedValue != null || value.Type == JTokenType.Null) + if (convertedValue != null || value.Type == JTokenType.Null) { - finalFieldInfo.SetValue(currentObject, convertedValue); + if (!Equals(finalFieldInfo.GetValue(currentObject), convertedValue)) + finalFieldInfo.SetValue(currentObject, convertedValue); return true; }
2070-2080: Use ordinal comparison for namespace prefix check.String.StartsWith without a comparison can be culture-sensitive. Use Ordinal.
- targetType.FullName.StartsWith("UnityEngine.U2D") || // Sprites etc. + targetType.FullName.StartsWith("UnityEngine.U2D", StringComparison.Ordinal) || // Sprites etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef(1 hunks)UnityMcpBridge/Editor/Tools/ManageAsset.cs(4 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.812Z
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-09-03T16:00:55.812Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.812Z
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:
UnityMcpBridge/Editor/Tools/ManageAsset.csUnityMcpBridge/Editor/Tools/ManageGameObject.cs
🧬 Code graph analysis (2)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (2)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (4)
ManageGameObject(22-2173)Type(2158-2172)ComponentResolver(2179-2439)TryResolve(2189-2225)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (5)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
HandleCommand(111-284)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (9)
Vector3Converter(11-34)Vector2Converter(36-56)QuaternionConverter(58-84)ColorConverter(86-112)RectConverter(114-140)BoundsConverter(142-161)UnityEngineObjectConverter(164-265)UnityEngine(222-264)Vector3(25-33)UnityMcpBridge/Editor/Tools/ManageAsset.cs (2)
HandleCommand(45-110)ConvertJTokenToType(1157-1234)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)UnityMcpBridge/Editor/Helpers/Vector3Helper.cs (1)
Vector3(17-22)
🔇 Additional comments (5)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (3)
10-10: Keep the static using (required to reach nested ComponentResolver).This is correct given ComponentResolver is a nested static type inside ManageGameObject. Avoid removing it.
205-215: Good switch to resolver with clear error surface.Replacing ad-hoc type lookup with ComponentResolver.TryResolve and echoing the resolver error in the message improves diagnostics for ScriptableObject creation.
953-955: Operator-precedence fix for float property guard is correct.The new grouping ensures a non-empty name and numeric value are both required.
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
24-37: Shared JsonSerializer with converters: good allocation and consistency win.Centralizing converters and reusing a single serializer reduces GC and keeps conversions uniform across paths.
2155-2172: Good: FindType now delegates to resolver and logs resolver context.This aligns the whole toolchain on the centralized resolution logic and surfaces ambiguity/not-found hints.
…perations - Change from fail-fast to collect-and-continue at component iteration level - Previously: first component error would halt processing of remaining components - Now: all components are processed, errors collected and returned together - Maintain existing collect-and-continue behavior within individual components - Add comprehensive tests validating the collect-and-continue behavior works correctly - All valid properties are applied even when invalid ones fail - Processing continues through exceptions with proper error aggregation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Tip For best results, initiate chat on the files or code changes. @dsarno Thank you for the correction! You're absolutely right about the The automated review comment incorrectly assumed ComponentResolver was a sibling type in the same namespace. Your clarification about the nested class architecture and the resulting compilation errors (CS0117) without the static import is spot on. I appreciate you taking the time to correct this - it helps improve the accuracy of future reviews! 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
787-813: Good: collect-and-continue on component property settingThis addresses the earlier early-return feedback and improves UX.
🧹 Nitpick comments (24)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (17)
1-1: Avoid file-wide nullable suppressionReplace the global #nullable disable with local, targeted pragmas where needed, or better, enable nullable and annotate. Keeps null-safety intact across the file.
-#nullable disable +// Consider removing global nullable suppression. Prefer local pragmas or proper annotations.
24-37: Shared JsonSerializer: good reuse; add minor safety tweaksLGTM on centralizing converters. Consider:
- Marking FloatParseHandling = FloatParseHandling.Double and Culture to Invariant for deterministic numeric parsing.
- Making the serializer Lazy<> to guard static init order and potential editor domain reload quirks (optional).
- private static readonly JsonSerializer InputSerializer = JsonSerializer.Create(new JsonSerializerSettings + private static readonly Lazy<JsonSerializer> _inputSerializer = new(() => JsonSerializer.Create(new JsonSerializerSettings { Converters = new List<JsonConverter> { new Vector3Converter(), new Vector2Converter(), new QuaternionConverter(), new ColorConverter(), new RectConverter(), new BoundsConverter(), new UnityEngineObjectConverter() - } - }); + }, + FloatParseHandling = FloatParseHandling.Double, + Culture = System.Globalization.CultureInfo.InvariantCulture, + })); + private static JsonSerializer InputSerializer => _inputSerializer.Value;
56-57: Support both 'searchMethod' and 'search_method'Tests and callers sometimes send snake_case. Read both keys to avoid silent fallback.
- string searchMethod = @params["searchMethod"]?.ToString().ToLower(); + string searchMethod = (@params["searchMethod"] ?? @params["search_method"])?.ToString().ToLower();
193-231: Prefab name search: use name: filter for precisionUse AssetDatabase name: filter to avoid substring matches and reduce ambiguity.
- string[] guids = AssetDatabase.FindAssets($"t:Prefab {prefabNameOnly}"); + string[] guids = AssetDatabase.FindAssets($"t:Prefab name:{prefabNameOnly}");
302-317: Minor: validate name before creating primitiveAvoid creating then immediately destroying when name is missing.
- newGo = GameObject.CreatePrimitive(type); - // Set name *after* creation for primitives - if (!string.IsNullOrEmpty(name)) - { - newGo.name = name; - } - else - { - UnityEngine.Object.DestroyImmediate(newGo); // cleanup leak - return Response.Error("'name' parameter is required when creating a primitive."); - } + if (string.IsNullOrEmpty(name)) + return Response.Error("'name' parameter is required when creating a primitive."); + newGo = GameObject.CreatePrimitive(type); + newGo.name = name;
374-385: Deduplicate ParseVector3: reuse Vector3HelperYou already have Helpers.Vector3Helper.ParseVector3. Remove the local helper and call the shared one for consistency.
- Vector3? position = ParseVector3(@params["position"] as JArray); - Vector3? rotation = ParseVector3(@params["rotation"] as JArray); - Vector3? scale = ParseVector3(@params["scale"] as JArray); + Vector3? position = MCPForUnity.Editor.Helpers.Vector3Helper.ParseVector3(@params["position"] as JArray); + Vector3? rotation = MCPForUnity.Editor.Helpers.Vector3Helper.ParseVector3(@params["rotation"] as JArray); + Vector3? scale = MCPForUnity.Editor.Helpers.Vector3Helper.ParseVector3(@params["scale"] as JArray);- /// <summary> - /// Parses a JArray like [x, y, z] into a Vector3. - /// </summary> - private static Vector3? ParseVector3(JArray array) - { - if (array != null && array.Count == 3) - { - try - { - return new Vector3( - array[0].ToObject<float>(), - array[1].ToObject<float>(), - array[2].ToObject<float>() - ); - } - catch (Exception ex) - { - Debug.LogWarning($"Failed to parse JArray as Vector3: {array}. Error: {ex.Message}"); - } - } - return null; - } + // Removed: use Vector3Helper.ParseVector3 insteadAlso applies to: 1141-1162
814-822: Consider returning partial-success instead of hard errorCurrently any component error flips the whole modify to Response.Error. Consider returning Success with a data.errors array for partial success, or include a status field, to better signal that some changes did apply.
Would you prefer Success with { warnings/errors } when at least one property applied?
913-984: Reverse-iterate + Insert(0, ...) is O(n^2)Since you copied the array, just iterate forward and Add to maintain order. This reduces allocations and shifts.
- for (int i = componentCount - 1; i >= 0; i--) - { - Component c = componentsToIterate[i]; - ... - if (data != null) - { - componentData.Insert(0, data); - } - ... - componentData.Insert(0, new JObject(...)); - } + for (int i = 0; i < componentCount; i++) + { + Component c = componentsToIterate[i]; + ... + if (data != null) + { + componentData.Add(data); + } + ... + componentData.Add(new JObject(...)); + }
1524-1584: Differentiate “not found” from “conversion failed”SetProperty returns false for multiple reasons; emitting “Property not found” on conversion errors is misleading (see velocity test). Return a reason enum to tailor messages.
- bool setResult = SetProperty(targetComponent, propName, propValue); - if (!setResult) + var setResult = SetProperty(targetComponent, propName, propValue, out var reason); + if (!setResult) { - var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); - var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, availableProperties); - var msg = suggestions.Any() - ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" - : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + string msg; + if (reason == SetFailureReason.NotFound) + { + var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); + var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, availableProperties); + msg = suggestions.Any() + ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" + : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + } + else + { + msg = $"Failed to convert value for '{propName}'. See previous errors for details."; + } Debug.LogWarning($"[ManageGameObject] {msg}"); failures.Add(msg); }Add this minimal helper near SetProperty:
+ private enum SetFailureReason { Unknown, NotFound, ConversionFailed }And adjust SetProperty signature to set reason accordingly.
1591-1661: Tighten NonPublic field lookup: drop IgnoreCaseCase-insensitive matching on private fields increases risk of touching the wrong backing field; keep IgnoreCase for public, not for NonPublic.
- var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase); + var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance);
1669-1865: Nested property setter: solid coverage; add IList/arrays beyond MaterialThe traversal is robust. Consider supporting typed arrays beyond Material[] (e.g., Component[], Vector4[]), since you already handle IList.
1907-1946: Exception rethrow policyConvertJTokenToType rethrows after logging; SetProperty catches and logs again, leading to double logs. Either don’t rethrow here or suppress the outer log on JsonSerializationException to reduce noise.
- catch (JsonSerializationException jsonEx) - { - Debug.LogError(...); - throw; - } + catch (JsonSerializationException jsonEx) + { + Debug.LogError(...); + return null; // Let caller handle a null conversion result. + }
2173-2219: TryResolve: case-sensitivity and UXCurrent Ordinal match enforces exact case. Consider OrdinalIgnoreCase for short names to be forgiving during interactive use.
- if (CacheByFqn.TryGetValue(nameOrFullName, out type)) return true; - if (!nameOrFullName.Contains(".") && CacheByName.TryGetValue(nameOrFullName, out type)) return true; + if (CacheByFqn.TryGetValue(nameOrFullName, out type)) return true; + if (!nameOrFullName.Contains(".")) + { + if (CacheByName.TryGetValue(nameOrFullName, out type)) return true; + if (CacheByName.TryGetValue(nameOrFullName, out type, StringComparer.OrdinalIgnoreCase)) return true; // optional helper overload + }Alternatively, normalize keying to OrdinalIgnoreCase for CacheByName.
2234-2271: Cache player assembly namesCompilationPipeline.GetAssemblies is relatively heavy; cache the player assembly name set.
+#if UNITY_EDITOR + private static readonly Lazy<HashSet<string>> PlayerAsmNames = new(() => + new HashSet<string>( + CompilationPipeline.GetAssemblies(AssembliesType.Player).Select(a => a.name), + StringComparer.Ordinal)); +#endif ... - var playerAsmNames = new HashSet<string>( - UnityEditor.Compilation.CompilationPipeline.GetAssemblies(UnityEditor.Compilation.AssembliesType.Player).Select(a => a.name), - StringComparer.Ordinal); - IEnumerable<System.Reflection.Assembly> playerAsms = loaded.Where(a => playerAsmNames.Contains(a.GetName().Name)); + IEnumerable<System.Reflection.Assembly> playerAsms = loaded.Where(a => PlayerAsmNames.Value.Contains(a.GetName().Name)); IEnumerable<System.Reflection.Assembly> editorAsms = loaded.Except(playerAsms); ... - var player = new HashSet<string>( - UnityEditor.Compilation.CompilationPipeline.GetAssemblies(UnityEditor.Compilation.AssembliesType.Player).Select(a => a.name), - StringComparer.Ordinal); - return seq.OrderBy(t => player.Contains(t.Assembly.GetName().Name) ? 0 : 1); + return seq.OrderBy(t => PlayerAsmNames.Value.Contains(t.Assembly.GetName().Name) ? 0 : 1);Also applies to: 2274-2282
2294-2312: GetAllComponentProperties: consider caching per typeCompute-once-and-cache per Type to avoid repeated reflection during suggestion/error flows.
2316-2355: Remove unused 'prompt' constructionThe prompt string is built then discarded. Drop it to reduce GC pressure.
- var prompt = $"A Unity developer is trying to set a component property but used an incorrect name.\n\n" + - ... - $"- \"Move Speed\" → \"moveSpeed, movementSpeed\""; - // For now, we'll use a simple rule-based approach that mimics AI behavior // This can be replaced with actual AI calls later var suggestions = GetRuleBasedSuggestions(userInput, availableProperties);
2429-2430: Comment contradicts reality“Removed duplicate ParseVector3 - using the one at line 1114” but a local ParseVector3 still exists above. After dedup (see earlier comment), update this note or remove it.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (7)
33-41: Assert Response shape, not just non-nullConvert the result to JObject and assert success == false for error cases to make tests meaningful.
- Assert.IsNotNull(result, "Should return a result object"); + var jo = JObject.FromObject(result); + Assert.IsFalse((bool)jo["success"], "Should be an error response"); + StringAssert.Contains("cannot be null", (string)jo["error"]);
42-50: Same as above: verify error on empty paramsAlso assert the specific error message ("Action parameter is required.").
- Assert.IsNotNull(result, "Should return a result object for empty params"); + var jo = JObject.FromObject(result); + Assert.IsFalse((bool)jo["success"]); + StringAssert.Contains("Action parameter is required", (string)jo["error"]);
95-110: Built-ins loop: nice; handle abstract types commentLGTM. You might also include “MeshRenderer” to cover concrete subclasses.
176-197: Cache perf test: reduce flakinessUse a more lenient ratio or assert secondCallTime <= firstCallTime + epsilon. Editor environments can be noisy.
- Assert.LessOrEqual(secondCallTime, firstCallTime * 2, "Cached call should not be significantly slower"); + Assert.LessOrEqual(secondCallTime, firstCallTime * 3 + 1.0, "Cached call should not be materially slower");
199-227: Use 'searchMethod' to match APIYou pass search_method but the handler consumes searchMethod. Either update tests or rely on alias support (suggested in code). Updating tests keeps them stricter.
- ["search_method"] = "by_name", + ["searchMethod"] = "by_name",Also applies to: 276-290
235-247: Drop noisy debug logs in testsThe BEFORE/AFTER Debug.Log lines add noise without assertions. Remove to keep logs clean.
- Debug.Log($"BEFORE TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}"); ... - Debug.Log($"AFTER TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}"); - Debug.Log($"AFTER TEST - LocalPosition: {transform.localPosition}"); - Debug.Log($"AFTER TEST - LocalScale: {transform.localScale}");
292-300: Log expectations: robust but brittle regexConsider anchoring patterns less tightly or asserting counts to avoid coupling to message wording if you adopt the “conversion failed” vs “not found” refinement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(1 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.812Z
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-09-03T16:00:55.812Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.812Z
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:
UnityMcpBridge/Editor/Tools/ManageGameObject.cs
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (14)
UnityEngine(2048-2145)GameObject(1167-1190)ManageGameObject(22-2167)HandleCommand(41-174)ComponentResolver(2173-2433)TryResolve(2183-2219)Type(2152-2166)List(1195-1354)List(2234-2271)List(2294-2312)List(2317-2356)List(2364-2399)Vector3(1144-1162)Vector3(1951-1966)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (6)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
System(2267-2333)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (4)
Vector3Converter(11-34)UnityEngineObjectConverter(164-265)UnityEngine(222-264)Vector3(25-33)UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
ConvertJTokenToType(1157-1234)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Error(41-60)UnityMcpBridge/Editor/Helpers/Vector3Helper.cs (1)
Vector3(17-22)UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs (1)
JToken(506-525)
🔇 Additional comments (6)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
2149-2166: FindType: good delegation and context loggingClean pass-through to ComponentResolver with helpful warnings. LGTM.
2363-2398: Nice: normalization + Levenshtein with 3-suggestion capSolid, pragmatic heuristics here. LGTM.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (4)
72-81: Resolver integration: good coverageDirectly validating Transform resolution and error being empty is perfect.
112-128: AI suggestions test: good signalExercising “Use Gravity” → “useGravity” validates normalization. Looks good.
129-138: Property set for MonoBehaviour: OKCovers base Component properties. LGTM.
140-159: Case variations table: concise and valuableGood data-driven coverage across cases/separators. LGTM.
1 participant
Summary
Basically, now the MCP can intelligentaly add components to objects and not get confused by assembly friction. This PR enhances Unity's ComponentResolver with robust assembly loading and intelligent property name matching to eliminate "Assembly-CSharp-Editor" errors and improve user experience.
Key Features
🔧 Robust Component Resolution
CompilationPipeline.GetAssemblies🎯 Intelligent Property Matching
✅ Comprehensive Testing
Technical Implementation
CompilationPipeline.GetAssemblies(AssembliesType.Player)instead ofAssembly.LoadFromExample Usage
Before:
[ManageGameObject] Could not set property 'Max Reach Distance' on component
'LongUnityScriptClaudeTest'
After:
Property 'Max Reach Distance' not found on LongUnityScriptClaudeTest.
Did you mean: maxReachDistance?
Available properties: [maxReachDistance, maxHorizontalDistance, maxVerticalDistance, ...]
Files Changed
UnityMcpBridge/Editor/Tools/ManageGameObject.cs- Major ComponentResolver refactorUnityMcpBridge/Editor/AssemblyInfo.cs- Test assembly access configurationTestProjects/.../AIPropertyMatchingTests.cs- Comprehensive property matching testsTestProjects/.../ManageGameObjectTests.cs- Integration tests with real componentsTestProjects/.../ComponentResolverTests.cs.meta- Test metadataTesting
All 45 unit tests pass, including:
Breaking Changes
None - this is purely additive functionality that improves existing behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests