Fix misleading 'Property not found' error and add SerializedProperty fallback (#765)#766
Merged
dsarno merged 4 commits intoCoplayDev:betafrom Feb 17, 2026
Conversation
… add SerializedProperty fallback (CoplayDev#765) When setting component properties via reflection failed (e.g. array types, UdonSharp proxies), the error message falsely reported "Property 'X' not found. Did you mean: X?" — even though the property existed. The root cause was that SetProperty returned a bare bool with no error detail, so the caller interpreted every failure as "not found." Changes: - GameObjectComponentHelpers.SetProperty now returns error details via out parameter, distinguishing conversion failures from missing properties - ComponentOps.SetProperty falls back to SetViaSerializedProperty when reflection-based conversion fails, handling arrays and custom serialization - GameObjectComponentHelpers falls back to ComponentOps.SetProperty when its local reflection fails, gaining the same SerializedProperty support Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideImproves component property setting by distinguishing conversion failures from missing properties, adding a SerializedProperty-based fallback path (including from ManageGameObject), and adding regression tests for error reporting behavior. Sequence diagram for ManageGameObject property setting with reflection and SerializedProperty fallbacksequenceDiagram
participant Caller
participant GameObjectComponentHelpers
participant TargetComponent
participant LocalSetProperty
participant ComponentOps
participant SerializedPropertyPath
Caller->>GameObjectComponentHelpers: SetComponentPropertiesInternal(targetGo, componentType, properties)
loop for each property
GameObjectComponentHelpers->>LocalSetProperty: SetProperty(targetComponent, propName, propValue, out setError)
alt local reflection succeeds
LocalSetProperty-->>GameObjectComponentHelpers: true, setError = null
GameObjectComponentHelpers-->>Caller: property set (continue loop)
else local reflection fails
LocalSetProperty-->>GameObjectComponentHelpers: false, setError
GameObjectComponentHelpers->>ComponentOps: SetProperty(targetComponent, propName, propValue, out opsError)
alt ComponentOps uses reflection or SerializedProperty and succeeds
ComponentOps-->>GameObjectComponentHelpers: true, opsError = null
GameObjectComponentHelpers-->>Caller: property set (continue loop)
else ComponentOps fails
ComponentOps-->>GameObjectComponentHelpers: false, opsError
alt setError is non null and not contains not found
GameObjectComponentHelpers->>GameObjectComponentHelpers: msg = setError
else opsError is non null and not contains not found
GameObjectComponentHelpers->>GameObjectComponentHelpers: msg = opsError
else
GameObjectComponentHelpers->>TargetComponent: query properties via ComponentResolver
GameObjectComponentHelpers->>GameObjectComponentHelpers: build not found message with suggestions
end
GameObjectComponentHelpers-->>Caller: warn and record msg
end
end
end
Class diagram for ComponentOps and GameObjectComponentHelpers property setting changesclassDiagram
class GameObjectComponentHelpers {
<<static>>
+SetComponentPropertiesInternal(targetGo, componentType, properties) object
-InputSerializer JsonSerializer
-SetProperty(target, memberName, value, error) bool
}
class ComponentOps {
<<static>>
+SetProperty(component, propertyName, value, error) bool
-TrySetViaReflection(component, type, propertyName, normalizedName, flags, value, error) bool
-SetViaSerializedProperty(component, propertyName, normalizedName, value, error) bool
-FindSerializedFieldInHierarchy(type, propertyName) FieldInfo
}
GameObjectComponentHelpers ..> ComponentOps : uses
GameObjectComponentHelpers ..> JsonSerializer : uses
ComponentOps ..> PropertyConversion : uses
ComponentOps ..> SerializedProperty : uses
ComponentOps ..> FieldInfo : uses
ComponentOps ..> Type : uses
ComponentOps ..> Component : operates on
Flow diagram for ComponentOps.SetProperty with reflection and SerializedProperty fallbackflowchart TD
A["SetProperty(component, propertyName, value, out error)"] --> B["Normalize propertyName"]
B --> C{UseReflection is true?}
C -->|yes| D["TrySetViaReflection(component, type, propertyName, normalizedName, flags, value, out error)"]
C -->|no| E["SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)"]
D -->|success| F["return true"]
D -->|failure| G["reflectionError = error"]
G --> H["SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)"]
H -->|success| F
H -->|failure| I{reflectionError not null and not contains not found?}
I -->|yes| J["error = reflectionError"]
I -->|no| K["keep SerializedProperty error"]
J --> L["return false"]
K --> L["return false"]
E -->|success| F
E -->|failure| L
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughTry reflection-first when setting component properties (PropertyInfo, FieldInfo, inherited/private Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as ManageGameObject
participant Helper as GameObjectComponentHelpers
participant Ops as ComponentOps (Reflection)
participant UnitySP as Unity SerializedProperty
rect rgba(200,200,255,0.5)
Caller->>Helper: SetComponentPropertiesInternal(command)
end
Helper->>Helper: Parse property path (nested vs simple)
alt nested path
Helper->>Helper: Traverse path, resolve intermediates (props/fields/serialized fields)
alt traversal success
Helper->>Ops: TrySetViaReflection(finalTarget, member, value)
else traversal failure
Helper-->>Caller: return detailed path error
end
else simple property
Helper->>Ops: TrySetViaReflection(component, member, value)
end
alt Reflection success
Ops-->>Helper: applied
Helper-->>Caller: success
else Reflection found member but conversion failed
Ops-->>Helper: conversion error (takes precedence)
Helper-->>Caller: surface conversion error
else Reflection did not find member
Ops-->>Helper: not found
Helper->>UnitySP: Attempt via SerializedProperty API
alt SerializedProperty success
UnitySP-->>Helper: applied
Helper-->>Caller: success
else SerializedProperty failure
UnitySP-->>Helper: not found/error
Helper-->>Caller: not-found (with suggestions)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Both
GameObjectComponentHelpers.SetComponentPropertiesInternalandComponentOps.SetPropertyrely onerror.Contains("not found")string checks to decide which message to surface; consider introducing a structured error type/enum or a dedicated flag to distinguish "missing member" vs "conversion" cases instead of brittle string matching. - The reflection-based property/field setting logic in
GameObjectComponentHelpers.SetPropertyandComponentOps.TrySetViaReflectionis now quite similar but duplicated; consider consolidating this into a shared helper to reduce divergence in behavior and future maintenance overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `GameObjectComponentHelpers.SetComponentPropertiesInternal` and `ComponentOps.SetProperty` rely on `error.Contains("not found")` string checks to decide which message to surface; consider introducing a structured error type/enum or a dedicated flag to distinguish "missing member" vs "conversion" cases instead of brittle string matching.
- The reflection-based property/field setting logic in `GameObjectComponentHelpers.SetProperty` and `ComponentOps.TrySetViaReflection` is now quite similar but duplicated; consider consolidating this into a shared helper to reduce divergence in behavior and future maintenance overhead.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs:186` </location>
<code_context>
+ }
+
+ // Both paths failed. Prefer the more specific error.
+ string msg = setError ?? opsError;
+ if (msg == null || msg.Contains("not found"))
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `opsError` outside of its declaration scope will not compile.
Declare `opsError` before the `if` so its scope includes this code, e.g. `string opsError = null;` above, then call `ComponentOps.SetProperty(..., out opsError)` inside the `if` block.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs:270-274` </location>
<code_context>
catch (Exception ex)
{
- McpLog.Error($"[SetProperty] Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}");
+ error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}";
+ return false;
}
</code_context>
<issue_to_address>
**suggestion:** Error message in the catch block loses useful context that was previously logged.
The previous log included the token contents and full message, which are important for diagnosing conversion issues. Now only `ex.Message` is surfaced. Since this is the only information propagated to the caller’s warning log, please include at least the token (e.g. `value.ToString(Formatting.None)`) or member/value details in the `error` string so the higher-level log stays actionable.
```suggestion
catch (Exception ex)
{
error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}";
return false;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs
Outdated
Show resolved
Hide resolved
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (3)
225-228:erroris not set whenSetNestedPropertyreturnsfalse.If
SetNestedPropertyfails,errorremainsnull(from line 216). The caller inSetComponentPropertiesInternalhandles this gracefully (falls through toComponentOps.SetPropertyand then to the "not found" suggestion path), so this isn't a bug. However, the nested-path errors logged internally bySetNestedPropertyviaMcpLog.Warn/McpLog.Errorare lost from the structured error response.Consider propagating a meaningful error from
SetNestedPropertyinto theout errorparameter so the caller can surface it to the user, rather than only logging it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 225 - 228, SetNestedProperty currently returns false on failure but doesn't populate the out error used by SetComponentPropertiesInternal, so nested-path warnings/errors logged via McpLog.Warn/McpLog.Error are lost; update SetNestedProperty (or its callers) to set the out string error before returning false (either by changing SetNestedProperty signature to bool SetNestedProperty(..., out string error) or by having it call a helper to produce a descriptive error and assign the existing out error variable prior to returning) so that when SetNestedProperty returns false the caller receives a meaningful error message that can be surfaced instead of only relying on McpLog logs.
175-196: Duplicate reflection pass produces redundant error logs.When the local
SetProperty(line 175) finds a member but fails conversion, it returnsfalsewith a conversion error. The fallback on line 180 then callsComponentOps.SetProperty, which re-runs the same reflection+conversion attempt viaTrySetViaReflectionbefore reaching the usefulSetViaSerializedPropertypath. This causes the same conversion error to be logged twice (confirmed by the test expecting two"Error converting token"log lines).Consider either:
- Having the local
SetPropertyset a flag indicating whether the member was found (so the fallback can skip straight toSetViaSerializedProperty), or- Removing the local reflection logic entirely and delegating to
ComponentOps.SetPropertywhich already covers reflection + SerializedProperty in a single pass.This would eliminate the double error logs and the near-duplicate reflection code between the two classes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 175 - 196, The local SetProperty method performs reflection and conversion which can produce conversion errors that are then re-run and logged again by ComponentOps.SetProperty (which calls TrySetViaReflection before SetViaSerializedProperty); to fix this, change SetProperty (or its callers) to communicate whether the member was found versus a conversion failure (e.g., return a result struct or an out bool memberFound) so the fallback path in this file can skip reflection and call ComponentOps.SetProperty in a mode that directly invokes SetViaSerializedProperty (or add an overload/flag on ComponentOps.SetProperty to skip TrySetViaReflection), or alternatively remove the local reflection logic and delegate entirely to ComponentOps.SetProperty; update call sites to use the new memberFound/flag/overload so conversion errors are not duplicated.
256-268: Non-public serialized field search doesn't traverse the inheritance hierarchy.
type.GetField(...)withBindingFlags.NonPubliconly returns fields declared directly on that type. If the[SerializeField]is declared on a base class, this lookup will miss it. By contrast,ComponentOps.TrySetViaReflectionusesFindSerializedFieldInHierarchywhich walks the full inheritance chain.This is functionally covered by the
ComponentOps.SetPropertyfallback on line 180, so it won't cause incorrect behavior — but it means this local branch is never reached for inherited private fields, making it slightly misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 256 - 268, The non-public serialized field lookup using type.GetField(...) only checks the current type and misses private [SerializeField] fields on base classes; update the search to traverse the inheritance chain by replacing the direct GetField call with a helper that walks base types (e.g., call the existing FindSerializedFieldInHierarchy or implement a loop that uses currentType.GetField(...) up the baseType chain) to locate the FieldInfo for memberName/normalizedName, then keep the existing SerializeField check, ConvertJTokenToType and npField.SetValue logic (referencing npField, ConvertJTokenToType, memberName and FindSerializedFieldInHierarchy/ComponentOps.TrySetViaReflection for locating the helper).MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
177-194: Well-structured two-path fallback with clear error precedence.The reflection-first, SerializedProperty-fallback approach correctly addresses the original issue. The error prioritization logic on lines 190–191 properly preserves conversion errors over generic "not found" messages.
One minor observation: the error precedence relies on substring matching (
reflectionError.Contains("not found")). This works today becauseTrySetViaReflectionuses a controlled "not found" phrase on line 269, but it's a fragile coupling. Consider using a dedicated sentinel (e.g., a booleanmemberFoundflag fromTrySetViaReflection) instead of string inspection to distinguish "member found but conversion failed" from "member not found." This would be more robust against future message changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 177 - 194, Replace the fragile string-inspection error precedence with an explicit sentinel from TrySetViaReflection: change TrySetViaReflection to return (or have an out parameter) like out bool memberFound (true when the member was located but conversion failed, false when not found), set that flag appropriately inside TrySetViaReflection, and then in the caller (where reflectionError is captured before calling SetViaSerializedProperty) use memberFound instead of reflectionError.Contains("not found") to decide whether to keep reflectionError or use the SerializedProperty error; update any other call sites of TrySetViaReflection accordingly (e.g., the code block around TrySetViaReflection, SetViaSerializedProperty, reflectionError) so that error precedence is determined by memberFound rather than substring matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs`:
- Around line 56-64: The tests are asserting against ErrorResponse.Error (the
top-level message) instead of the detailed failure messages stored in
ErrorResponse.Data.errors; update the assertions in
GameObjectComponentHelpersErrorTests.cs to inspect errorResponse.Data.errors
(cast/unpack it to the expected collection or dynamic) and assert that the
individual error strings contain "failed" for conversion failures and do NOT
contain "not found" for properties that exist, referencing the ErrorResponse
type and the Data.errors payload when locating where to change the checks.
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 177-194: Replace the fragile string-inspection error precedence
with an explicit sentinel from TrySetViaReflection: change TrySetViaReflection
to return (or have an out parameter) like out bool memberFound (true when the
member was located but conversion failed, false when not found), set that flag
appropriately inside TrySetViaReflection, and then in the caller (where
reflectionError is captured before calling SetViaSerializedProperty) use
memberFound instead of reflectionError.Contains("not found") to decide whether
to keep reflectionError or use the SerializedProperty error; update any other
call sites of TrySetViaReflection accordingly (e.g., the code block around
TrySetViaReflection, SetViaSerializedProperty, reflectionError) so that error
precedence is determined by memberFound rather than substring matching.
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 225-228: SetNestedProperty currently returns false on failure but
doesn't populate the out error used by SetComponentPropertiesInternal, so
nested-path warnings/errors logged via McpLog.Warn/McpLog.Error are lost; update
SetNestedProperty (or its callers) to set the out string error before returning
false (either by changing SetNestedProperty signature to bool
SetNestedProperty(..., out string error) or by having it call a helper to
produce a descriptive error and assign the existing out error variable prior to
returning) so that when SetNestedProperty returns false the caller receives a
meaningful error message that can be surfaced instead of only relying on McpLog
logs.
- Around line 175-196: The local SetProperty method performs reflection and
conversion which can produce conversion errors that are then re-run and logged
again by ComponentOps.SetProperty (which calls TrySetViaReflection before
SetViaSerializedProperty); to fix this, change SetProperty (or its callers) to
communicate whether the member was found versus a conversion failure (e.g.,
return a result struct or an out bool memberFound) so the fallback path in this
file can skip reflection and call ComponentOps.SetProperty in a mode that
directly invokes SetViaSerializedProperty (or add an overload/flag on
ComponentOps.SetProperty to skip TrySetViaReflection), or alternatively remove
the local reflection logic and delegate entirely to ComponentOps.SetProperty;
update call sites to use the new memberFound/flag/overload so conversion errors
are not duplicated.
- Around line 256-268: The non-public serialized field lookup using
type.GetField(...) only checks the current type and misses private
[SerializeField] fields on base classes; update the search to traverse the
inheritance chain by replacing the direct GetField call with a helper that walks
base types (e.g., call the existing FindSerializedFieldInHierarchy or implement
a loop that uses currentType.GetField(...) up the baseType chain) to locate the
FieldInfo for memberName/normalizedName, then keep the existing SerializeField
check, ConvertJTokenToType and npField.SetValue logic (referencing npField,
ConvertJTokenToType, memberName and
FindSerializedFieldInHierarchy/ComponentOps.TrySetViaReflection for locating the
helper).
Comment on lines
+56
to
+64
| Assert.IsNotNull(result, "Should return an error response"); | ||
| Assert.IsInstanceOf<ErrorResponse>(result); | ||
|
|
||
| var errorResponse = (ErrorResponse)result; | ||
|
|
||
| // The error message must NOT say "not found" for a property that exists | ||
| Assert.IsFalse( | ||
| errorResponse.Error.Contains("not found"), | ||
| $"Error should report conversion failure, not 'not found'. Got: {errorResponse.Error}"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the ErrorResponse class definition
fd -e cs -x grep -l "class ErrorResponse" {}Repository: CoplayDev/unity-mcp
Length of output: 103
🏁 Script executed:
# Search for ErrorResponse class definition with context
rg -n "class ErrorResponse" --type=cs -A 20Repository: CoplayDev/unity-mcp
Length of output: 1574
🏁 Script executed:
# Also search for any properties or methods that might concatenate error messages
rg -n "ErrorResponse" --type=cs -B 2 -A 10 | head -100Repository: CoplayDev/unity-mcp
Length of output: 8341
🏁 Script executed:
# Get the full ErrorResponse class definition
sed -n '35,80p' MCPForUnity/Editor/Helpers/Response.csRepository: CoplayDev/unity-mcp
Length of output: 1334
🏁 Script executed:
# Look for the ErrorResponse constructor
rg -n "public ErrorResponse" MCPForUnity/Editor/Helpers/Response.cs -A 15Repository: CoplayDev/unity-mcp
Length of output: 554
Test assertions do not validate the intended behavior — they check the wrong error property.
ErrorResponse.Error contains only the top-level message passed to the constructor:
new ErrorResponse($"One or more properties failed on '{componentTypeName}'.", new { errors = failures })Results in errorResponse.Error = "One or more properties failed on 'AudioSource'." The individual error messages live in Data.errors, not in the Error property.
Consequently:
- Test 1 (line 62):
!errorResponse.Error.Contains("not found")always passes because the top-level message never contains "not found" — regardless of whether conversion errors are actually reported correctly. - Test 2 (line 88):
errorResponse.Error.Contains("failed")always passes because the top-level message always contains "failed" — failing to distinguish between conversion failures and "not found" errors.
Both assertions should inspect the individual failure messages in Data.errors to properly verify that conversion failures report correctly and do not report "not found".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs`
around lines 56 - 64, The tests are asserting against ErrorResponse.Error (the
top-level message) instead of the detailed failure messages stored in
ErrorResponse.Data.errors; update the assertions in
GameObjectComponentHelpersErrorTests.cs to inspect errorResponse.Data.errors
(cast/unpack it to the expected collection or dynamic) and assert that the
individual error strings contain "failed" for conversion failures and do NOT
contain "not found" for properties that exist, referencing the ErrorResponse
type and the Data.errors payload when locating where to change the checks.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (1)
225-228:⚠️ Potential issue | 🟠 MajorNested-path failures silently leave
errorasnull, producing misleading "not found" messages.When
SetNestedPropertyreturnsfalse,errorstaysnull(initialized on line 216). The caller (line 187) treatsmsg == nullthe same as "not found" and generates a property-not-found message with suggestions — even though the property path was recognized and partially traversed. This is exactly the class of misleading error this PR aims to fix.Propagate a meaningful error from
SetNestedProperty(or seterrorhere on failure):Proposed fix
if (memberName.Contains('.') || memberName.Contains('[')) { - return SetNestedProperty(target, memberName, value, inputSerializer); + if (SetNestedProperty(target, memberName, value, inputSerializer)) + return true; + error = $"Failed to set nested property '{memberName}' on {type.Name}."; + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 225 - 228, When handling nested paths (the branch checking memberName.Contains('.') || memberName.Contains('[')), failure of SetNestedProperty currently returns false but leaves the local variable error null, causing the caller to emit a misleading "not found" message; fix this by propagating a meaningful error: either change SetNestedProperty to return/provide an error string (e.g., bool TrySetNestedProperty(..., out string? error)) and use that here, or set the local error variable when SetNestedProperty returns false with a clear message describing which part of memberName failed and why (referencing the SetNestedProperty routine and the memberName and error variables).
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (1)
175-196: Double reflection pass is redundant — consider callingComponentOps.SetPropertydirectly.The local
SetProperty(line 175) performs the same reflection resolution (public property → public field → non-public[SerializeField]field) thatComponentOps.SetProperty(line 180) repeats internally viaTrySetViaReflection. When the local attempt fails,ComponentOpsre-does the identical lookups before falling through toSerializedProperty.You could skip the local
SetPropertyentirely and callComponentOps.SetPropertyas the sole path, since it already surfaces conversion-vs-not-found errors through itsout string error. This would eliminate the redundant reflection and simplify this method to one call + the suggestion logic.Not blocking, since correctness is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 175 - 196, Remove the redundant local reflection pass (the local SetProperty call) and instead call ComponentOps.SetProperty(targetComponent, propName, propValue, out string opsError) as the single attempt; if it returns true continue, otherwise use opsError (prefer it over null) for the existing suggestion logic that uses ComponentResolver.GetAllComponentProperties and ComponentResolver.GetFuzzyPropertySuggestions, then McpLog.Warn the message and failures.Add(msg) as before. Ensure you preserve the fallback suggestion text construction and the preference for a more specific error message (use opsError when available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 225-228: When handling nested paths (the branch checking
memberName.Contains('.') || memberName.Contains('[')), failure of
SetNestedProperty currently returns false but leaves the local variable error
null, causing the caller to emit a misleading "not found" message; fix this by
propagating a meaningful error: either change SetNestedProperty to
return/provide an error string (e.g., bool TrySetNestedProperty(..., out string?
error)) and use that here, or set the local error variable when
SetNestedProperty returns false with a clear message describing which part of
memberName failed and why (referencing the SetNestedProperty routine and the
memberName and error variables).
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 175-196: Remove the redundant local reflection pass (the local
SetProperty call) and instead call ComponentOps.SetProperty(targetComponent,
propName, propValue, out string opsError) as the single attempt; if it returns
true continue, otherwise use opsError (prefer it over null) for the existing
suggestion logic that uses ComponentResolver.GetAllComponentProperties and
ComponentResolver.GetFuzzyPropertySuggestions, then McpLog.Warn the message and
failures.Add(msg) as before. Ensure you preserve the fallback suggestion text
construction and the preference for a more specific error message (use opsError
when available).
Address CodeRabbit review feedback: - Remove duplicated reflection logic from GameObjectComponentHelpers - Route non-nested property paths directly through ComponentOps.SetProperty - Add out string error to SetNestedProperty for proper error propagation - Fix MaterialOps.TrySetShaderProperty call to set error on failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (1)
278-302:⚠️ Potential issue | 🟡 MinorEdge case: confusing error message when array/list is empty.
When
materials.Lengthorlist.Countis 0, the error reads"out of range (0--1)"which is nonsensical. Handle the empty case separately.Proposed fix (example for the Material[] branch; apply similarly to IList)
var materials = currentObject as Material[]; if (arrayIndex < 0 || arrayIndex >= materials.Length) { - error = $"Material index {arrayIndex} out of range (0-{materials.Length - 1}) in path '{path}'."; + error = materials.Length == 0 + ? $"Material index {arrayIndex} out of range (array is empty) in path '{path}'." + : $"Material index {arrayIndex} out of range (0-{materials.Length - 1}) in path '{path}'."; return false; }var list = currentObject as System.Collections.IList; if (arrayIndex < 0 || arrayIndex >= list.Count) { - error = $"Index {arrayIndex} out of range (0-{list.Count - 1}) in path '{path}'."; + error = list.Count == 0 + ? $"Index {arrayIndex} out of range (list is empty) in path '{path}'." + : $"Index {arrayIndex} out of range (0-{list.Count - 1}) in path '{path}'."; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 278 - 302, The error messages for index-out-of-range when handling Material[] and System.Collections.IList produce "0--1" when the collection is empty; update the branches in the code that reference currentObject, Material[], materials, list, and arrayIndex to detect empty collections (materials.Length == 0 and list.Count == 0) and set a clear message like "collection is empty" (e.g., error = $"Cannot index into empty Materials in path '{path}'." and similarly for IList) instead of printing a negative upper bound; keep the existing bounds checks for non-empty cases and the same return false behavior, and update the error messages that include part and path accordingly.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (2)
175-203: Routing and error classification look correct for the PR's objective.The split between nested-path handling and
ComponentOps.SetPropertyfor simple names is clean and well-commented.One observation on the error classification at lines 193–200: using
msg.Contains("not found")for string-based dispatch is fragile — any conversion error that coincidentally contains the substring"not found"(e.g.,"Converter not found for type X") would be misclassified as a missing-property error and overwritten with fuzzy suggestions, masking the real cause.Consider using a structured result (enum or bool flag) from the setter instead of inspecting error-message text, or at minimum matching the exact known sentinel string rather than a substring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 175 - 203, The current logic misclassifies errors by checking msg.Contains("not found") which can match unrelated messages; change the setter APIs to return structured results instead of relying on substring matching (e.g., add an enum PropertySetResult { Success, NotFound, ConversionError } or similar and update SetNestedProperty and ComponentOps.SetProperty to return it or an out param), then branch on that enum to decide when to generate fuzzy suggestions via ComponentResolver.GetFuzzyPropertySuggestions and when to preserve the original setError; as a minimal alternative, introduce a single sentinel constant (e.g., PropertyNotFoundSentinel) returned explicitly by SetNestedProperty/ComponentOps.SetProperty and replace the Contains check with an equality check against that sentinel before calling ComponentResolver.GetAllComponentProperties and logging via McpLog.Warn and failures.Add.
320-346: Nested paths won't resolve[SerializeField] privatefields on the final segment.The
flags(line 234) useBindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase— this omitsNonPublic. While simple property names now go throughComponentOps.SetProperty(which has the SerializedProperty fallback), nested dot/bracket paths hitting a private[SerializeField]field on the final or intermediate segment will fall through to the "not found" error.This may be acceptable as a known limitation for now, but worth a comment or a follow-up item if nested
[SerializeField]access is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 320 - 346, The code that resolves the final path segment uses BindingFlags stored in flags (used by currentType.GetProperty/GetField) which omits NonPublic so private [SerializeField] fields are not found for nested paths; update the resolution logic in the final-segment block (where GetProperty/GetField and ConvertJTokenToType are used) to include BindingFlags.NonPublic (or explicitly attempt a second lookup with BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) so private serialized fields are discovered, and preserve the existing ConvertJTokenToType/currentObject handling and error assignment when conversion still fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 278-302: The error messages for index-out-of-range when handling
Material[] and System.Collections.IList produce "0--1" when the collection is
empty; update the branches in the code that reference currentObject, Material[],
materials, list, and arrayIndex to detect empty collections (materials.Length ==
0 and list.Count == 0) and set a clear message like "collection is empty" (e.g.,
error = $"Cannot index into empty Materials in path '{path}'." and similarly for
IList) instead of printing a negative upper bound; keep the existing bounds
checks for non-empty cases and the same return false behavior, and update the
error messages that include part and path accordingly.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 175-203: The current logic misclassifies errors by checking
msg.Contains("not found") which can match unrelated messages; change the setter
APIs to return structured results instead of relying on substring matching
(e.g., add an enum PropertySetResult { Success, NotFound, ConversionError } or
similar and update SetNestedProperty and ComponentOps.SetProperty to return it
or an out param), then branch on that enum to decide when to generate fuzzy
suggestions via ComponentResolver.GetFuzzyPropertySuggestions and when to
preserve the original setError; as a minimal alternative, introduce a single
sentinel constant (e.g., PropertyNotFoundSentinel) returned explicitly by
SetNestedProperty/ComponentOps.SetProperty and replace the Contains check with
an equality check against that sentinel before calling
ComponentResolver.GetAllComponentProperties and logging via McpLog.Warn and
failures.Add.
- Around line 320-346: The code that resolves the final path segment uses
BindingFlags stored in flags (used by currentType.GetProperty/GetField) which
omits NonPublic so private [SerializeField] fields are not found for nested
paths; update the resolution logic in the final-segment block (where
GetProperty/GetField and ConvertJTokenToType are used) to include
BindingFlags.NonPublic (or explicitly attempt a second lookup with
BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) so
private serialized fields are discovered, and preserve the existing
ConvertJTokenToType/currentObject handling and error assignment when conversion
still fails.
…paths Address CodeRabbit review round 2: - Handle empty arrays/lists separately to avoid nonsensical "out of range (0--1)" - Add [SerializeField] private field lookup for final segment of nested paths - Expose FindSerializedFieldInHierarchy as internal for cross-class reuse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
187-191: Fragile string-based error routing.The error-priority decision at line 190 hinges on
reflectionError.Contains("not found"). This couples the control flow to the exact wording produced byTrySetViaReflection(line 269). A future wording change (e.g., "was not found") would silently misroute the error.Consider using a structured signal (e.g., a boolean
memberFoundout-parameter fromTrySetViaReflection, or a small enum/result type) instead of parsing the error string.♻️ Suggested approach
- private static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, BindingFlags flags, JToken value, out string error) + private static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, BindingFlags flags, JToken value, out string error, out bool memberFound)Then in
SetProperty:- if (reflectionError != null && !reflectionError.Contains("not found")) + if (reflectionError != null && reflectionMemberFound) error = reflectionError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 187 - 191, The current error routing in SetProperty relies on parsing reflectionError text; change TrySetViaReflection to return a structured result (e.g., bool memberFound as an out parameter or a small enum/result type) instead of encoding presence in the message, update TrySetViaReflection's callers to accept the new return, and in SetProperty use that memberFound flag to decide whether to prefer reflectionError or the SerializedProperty error (replacing reflectionError.Contains("not found") with the explicit flag) so wording changes no longer affect control flow.MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs (1)
411-414:inputSerializerparameter is unused.
ConvertJTokenToTypeaccepts aJsonSerializerbut silently ignores it, delegating directly toPropertyConversion.ConvertToType. Every call site inSetNestedPropertypassesinputSerializer, giving the impression that serializer settings (e.g., custom converters) are honored when they are not.Either use the serializer in the conversion or remove the parameter to avoid misleading callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs` around lines 411 - 414, The ConvertJTokenToType method currently ignores the inputSerializer parameter; update it to honor custom serializer settings by using the serializer to convert the token (e.g., return token.ToObject(targetType, inputSerializer) or return inputSerializer.Deserialize(token.CreateReader(), targetType)) so callers from SetNestedProperty actually respect converters/settings; alternatively, if you intentionally do not want the serializer, remove the inputSerializer parameter from ConvertJTokenToType and all call sites in SetNestedProperty to avoid misleading API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 187-191: The current error routing in SetProperty relies on
parsing reflectionError text; change TrySetViaReflection to return a structured
result (e.g., bool memberFound as an out parameter or a small enum/result type)
instead of encoding presence in the message, update TrySetViaReflection's
callers to accept the new return, and in SetProperty use that memberFound flag
to decide whether to prefer reflectionError or the SerializedProperty error
(replacing reflectionError.Contains("not found") with the explicit flag) so
wording changes no longer affect control flow.
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs`:
- Around line 411-414: The ConvertJTokenToType method currently ignores the
inputSerializer parameter; update it to honor custom serializer settings by
using the serializer to convert the token (e.g., return
token.ToObject(targetType, inputSerializer) or return
inputSerializer.Deserialize(token.CreateReader(), targetType)) so callers from
SetNestedProperty actually respect converters/settings; alternatively, if you
intentionally do not want the serializer, remove the inputSerializer parameter
from ConvertJTokenToType and all call sites in SetNestedProperty to avoid
misleading API surface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Property 'X' not found. Did you mean: X?when setting component properties that exist but fail type conversion (e.g. arrays, UdonSharp proxy types)SerializedPropertyfallback inComponentOps.SetPropertywhen reflection-based conversion fails, enabling support for array properties (string[],int[],float[], etc.) and custom serialization systems like UdonSharpGameObjectComponentHelpers(theManageGameObjectcode path) now falls back toComponentOps.SetProperty, gaining the sameSerializedPropertysupportCloses #765
Test plan
string[],int[],float[], and[SerializeField] private string[]on a test component through bothmanage_componentsandmanage_gameobjectpaths — all values verified via resource read🤖 Generated with Claude Code
Summary by Sourcery
Improve component property setting to distinguish conversion failures from missing properties and to leverage SerializedProperty as a fallback for broader type support.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests