Skip to content

Commit 17ad011

Browse files
dsarnoclaude
dsarno
and
claude
committed
fix: Address CodeRabbit review issues and improve robustness
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 <[email protected]>
1 parent aac237c commit 17ad011

File tree

5 files changed

+59
-18
lines changed

5 files changed

+59
-18
lines changed

‎TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22
"name": "TestAsmdef",
33
"rootNamespace": "TestNamespace",
44
"references": [],
5-
"includePlatforms": [],
5+
"includePlatforms": ["Editor"],
66
"excludePlatforms": [],
77
"allowUnsafeCode": false,
88
"overrideReferences": false,
99
"precompiledReferences": [],
10-
"autoReferenced": true,
10+
"autoReferenced": false,
1111
"defineConstraints": [],
1212
"versionDefines": [],
1313
"noEngineReferences": false

‎TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPForUnityTests.Editor.asmdef‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
"rootNamespace": "",
44
"references": [
55
"MCPForUnity.Editor",
6+
"TestAsmdef",
67
"UnityEngine.TestRunner",
78
"UnityEditor.TestRunner"
89
],

‎TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,11 @@ public void TryResolve_ReturnsTrue_ForBuiltInComponentFullyQualifiedName()
3131
[Test]
3232
public void TryResolve_ReturnsTrue_ForCustomComponentShortName()
3333
{
34-
bool result = ComponentResolver.TryResolve("TicTacToe3D", out Type type, out string error);
34+
bool result = ComponentResolver.TryResolve("CustomComponent", out Type type, out string error);
3535

36-
Assert.IsTrue(result, "Should resolve TicTacToe3D component");
36+
Assert.IsTrue(result, "Should resolve CustomComponent");
3737
Assert.IsNotNull(type, "Should return valid type");
38-
Assert.AreEqual("TicTacToe3D", type.Name, "Should have correct type name");
38+
Assert.AreEqual("CustomComponent", type.Name, "Should have correct type name");
3939
Assert.IsTrue(typeof(Component).IsAssignableFrom(type), "Should be a Component type");
4040
Assert.IsEmpty(error, "Should have no error message");
4141
}

‎UnityMcpBridge/Editor/Tools/ManageAsset.cs‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using UnityEditor;
88
using UnityEngine;
99
using MCPForUnity.Editor.Helpers; // For Response class
10+
using static MCPForUnity.Editor.Tools.ManageGameObject; // For ComponentResolver
1011

1112
#if UNITY_6000_0_OR_NEWER
1213
using PhysicsMaterialType = UnityEngine.PhysicsMaterial;
@@ -207,9 +208,10 @@ private static object CreateAsset(JObject @params)
207208
|| !typeof(ScriptableObject).IsAssignableFrom(scriptType)
208209
)
209210
{
210-
return Response.Error(
211-
$"Script class '{scriptClassName}' not found or does not inherit from ScriptableObject."
212-
);
211+
var reason = scriptType == null
212+
? (string.IsNullOrEmpty(error) ? "Type not found." : error)
213+
: "Type found but does not inherit from ScriptableObject.";
214+
return Response.Error($"Script class '{scriptClassName}' invalid: {reason}");
213215
}
214216

215217
ScriptableObject so = ScriptableObject.CreateInstance(scriptType);
@@ -353,10 +355,18 @@ prop.Value is JObject componentProperties
353355
&& componentProperties.HasValues
354356
) // e.g., {"bobSpeed": 2.0}
355357
{
356-
// Find the component on the GameObject using the name from the JSON key
357-
// Using GetComponent(string) is convenient but might require exact type name or be ambiguous.
358-
// Consider using ComponentResolver if needed for more complex scenarios.
359-
Component targetComponent = gameObject.GetComponent(componentName);
358+
// Resolve component type via ComponentResolver, then fetch by Type
359+
Component targetComponent = null;
360+
if (ComponentResolver.TryResolve(componentName, out var compType, out var compError))
361+
{
362+
targetComponent = gameObject.GetComponent(compType);
363+
}
364+
else
365+
{
366+
Debug.LogWarning(
367+
$"[ManageAsset.ModifyAsset] Failed to resolve component '{componentName}' on '{gameObject.name}': {compError}"
368+
);
369+
}
360370

361371
if (targetComponent != null)
362372
{
@@ -937,8 +947,8 @@ private static bool ApplyMaterialProperties(Material mat, JObject properties)
937947
{
938948
string propName = floatProps["name"]?.ToString();
939949
if (
940-
!string.IsNullOrEmpty(propName) && floatProps["value"]?.Type == JTokenType.Float
941-
|| floatProps["value"]?.Type == JTokenType.Integer
950+
!string.IsNullOrEmpty(propName) &&
951+
(floatProps["value"]?.Type == JTokenType.Float || floatProps["value"]?.Type == JTokenType.Integer)
942952
)
943953
{
944954
try

‎UnityMcpBridge/Editor/Tools/ManageGameObject.cs‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#nullable enable
1+
#nullable disable
22
using System;
33
using System.Collections.Generic;
44
using System.Linq;
@@ -289,11 +289,16 @@ private static object CreateGameObject(JObject @params)
289289
newGo = GameObject.CreatePrimitive(type);
290290
// Set name *after* creation for primitives
291291
if (!string.IsNullOrEmpty(name))
292+
{
292293
newGo.name = name;
294+
}
293295
else
296+
{
297+
UnityEngine.Object.DestroyImmediate(newGo); // cleanup leak
294298
return Response.Error(
295299
"'name' parameter is required when creating a primitive."
296-
); // Name is essential
300+
);
301+
}
297302
createdNewObject = true;
298303
}
299304
catch (ArgumentException)
@@ -1493,7 +1498,18 @@ private static object SetComponentPropertiesInternal(
14931498
Component targetComponentInstance = null
14941499
)
14951500
{
1496-
Component targetComponent = targetComponentInstance ?? targetGo.GetComponent(compName);
1501+
Component targetComponent = targetComponentInstance;
1502+
if (targetComponent == null)
1503+
{
1504+
if (ComponentResolver.TryResolve(compName, out var compType, out var compError))
1505+
{
1506+
targetComponent = targetGo.GetComponent(compType);
1507+
}
1508+
else
1509+
{
1510+
targetComponent = targetGo.GetComponent(compName); // fallback to string-based lookup
1511+
}
1512+
}
14971513
if (targetComponent == null)
14981514
{
14991515
return Response.Error(
@@ -1627,6 +1643,20 @@ private static bool SetProperty(object target, string memberName, JToken value)
16271643
Debug.LogWarning($"[SetProperty] Conversion failed for field '{memberName}' (Type: {fieldInfo.FieldType.Name}) from token: {value.ToString(Formatting.None)}");
16281644
}
16291645
}
1646+
else
1647+
{
1648+
// Try NonPublic [SerializeField] fields
1649+
var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase);
1650+
if (npField != null && npField.GetCustomAttribute<SerializeField>() != null)
1651+
{
1652+
object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer);
1653+
if (convertedValue != null || value.Type == JTokenType.Null)
1654+
{
1655+
npField.SetValue(target, convertedValue);
1656+
return true;
1657+
}
1658+
}
1659+
}
16301660
}
16311661
}
16321662
catch (Exception ex)
@@ -2199,7 +2229,7 @@ private static bool NamesMatch(Type t, string q) =>
21992229
t.Name.Equals(q, StringComparison.Ordinal) ||
22002230
(t.FullName?.Equals(q, StringComparison.Ordinal) ?? false);
22012231

2202-
private static bool IsValidComponent(Type? t) =>
2232+
private static bool IsValidComponent(Type t) =>
22032233
t != null && typeof(Component).IsAssignableFrom(t);
22042234

22052235
private static void Cache(Type t)

0 commit comments

Comments
 (0)