fix: replace Editor-only McpLog with Debug.LogWarning in Runtime asse…#535
Merged
Merged
Conversation
…mbly - UnityTypeConverters.cs referenced McpLog (Editor-only) from Runtime asmdef - This caused CS0103 build errors in player builds - Replaced with UnityEngine.Debug.LogWarning for runtime compatibility Also cleaned up test file: - Removed stale NL test artifacts (Build marker, Tail test comments) - Removed unused local functions causing CS8321 warnings
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces an Editor-only logging call with a runtime-safe Debug.LogWarning in the Unity runtime assembly to fix player build errors, and cleans up obsolete test artifacts and unused local functions in a long test script to remove compiler warnings. Sequence diagram for runtime deserialization warning in non-Editor modesequenceDiagram
participant Caller
participant UnityEngineObjectConverter
participant UnityEngineDebug
Caller->>UnityEngineObjectConverter: ReadJson(reader, objectType, existingValue)
UnityEngineObjectConverter->>UnityEngineObjectConverter: Detect nonEditor runtime context
UnityEngineObjectConverter->>UnityEngineDebug: LogWarning("UnityEngineObjectConverter cannot deserialize complex objects in non-Editor mode.")
UnityEngineObjectConverter->>UnityEngineObjectConverter: Skip unsupported token
UnityEngineObjectConverter-->>Caller: Return null or existingValue
Updated class diagram for UnityEngineObjectConverter logging behaviorclassDiagram
class UnityEngineObjectConverter {
+ReadJson(reader, objectType, existingValue) UnityEngine.Object
-HandleRuntimeDeserialization(reader) void
}
class UnityEngineDebug {
+LogWarning(message) void
}
UnityEngineObjectConverter ..> UnityEngineDebug : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis change fixes a bug where Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
346-349: Consider simplifying token consumption.The token consumption logic works, but could be more explicit and defensive.
♻️ Optional: More explicit token consumption
UnityEngine.Debug.LogWarning("UnityEngineObjectConverter cannot deserialize complex objects in non-Editor mode."); // Skip the token to avoid breaking the reader -if (reader.TokenType == JsonToken.StartObject) JObject.Load(reader); -else if (reader.TokenType == JsonToken.String) reader.ReadAsString(); +if (reader.TokenType == JsonToken.StartObject) +{ + JObject.Load(reader); // Consume entire object +} +else +{ + reader.Read(); // Consume any other token (typically String) +} // Return null or existing value, depending on desired behavior return existingValue;This approach:
- Uses
reader.Read()for simple token consumption without processing the value- Handles any unexpected token type defensively (though only String/StartObject are expected)
- More explicit about the intention to skip/consume
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.csTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
💤 Files with no reviewable changes (1)
- TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
🧬 Code graph analysis (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (2)
UnityEngine(32-105)UnityEngine(170-199)MCPForUnity/Editor/Helpers/PropertyConversion.cs (1)
UnityEngine(76-90)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
344-344: Correct fix for the runtime build error.Replacing
McpLog.WarnwithUnityEngine.Debug.LogWarningsuccessfully resolves the CS0103 error by using a runtime-compatible logging API. The warning message clearly communicates the limitation.
This was referenced Apr 9, 2026
Merged
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.
…mbly
fixes #533
Also cleaned up test file:
Summary by Sourcery
Replace editor-only logging with runtime-safe warnings and clean up obsolete test artifacts.
Bug Fixes:
Tests:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.