Skip to content

Conversation

@dsarno
Copy link
Collaborator

@dsarno dsarno commented Jan 19, 2026

Summary

Fixes #585 - UIDocument component causes infinite loop when serializing components.

  • Adds special handling for UIDocument in GameObjectSerializer.GetComponentData(), similar to existing handling for Transform and Camera
  • Safely serializes panelSettings, visualTreeAsset, sortingOrder, enabled, parentUI properties
  • Explicitly skips rootVisualElement to prevent circular parent/child reference loops
  • Includes comprehensive test coverage

Root Cause

The UIDocument.rootVisualElement property returns a VisualElement which has circular parent/child references (children[] → child elements → parent → back to parent). The generic reflection-based serializer follows these references, causing infinite loops or massive payload dumps.

Test Plan

  • Added UIDocumentSerializationTests.cs with 5 test cases covering:
    • Serialization with both assets assigned
    • Serialization with only panelSettings
    • Serialization with only visualTreeAsset
    • Serialization with no assets
    • Verification of expected properties in output
  • All 267 EditMode tests pass on Unity 2021.3
  • Manually verified fix works on Unity 6.3 (ramble project)

Before/After

Before (Unity 6.3 without fix): ~200+ lines of nested VisualElement data dumped

After (with fix):

{
  "typeName": "UnityEngine.UIElements.UIDocument",
  "instanceID": -6123560,
  "properties": {
    "panelSettings": null,
    "visualTreeAsset": null,
    "sortingOrder": 0.0,
    "enabled": true,
    "parentUI": null,
    "_note": "rootVisualElement skipped to prevent circular reference loops"
  }
}

Summary by Sourcery

Add special-case serialization for UIDocument components to avoid infinite loops and improve GameObject modification behavior when activating inactive objects.

New Features:

  • Provide explicit serialization output for UIDocument components, including key properties and related assets while omitting the root visual hierarchy.

Bug Fixes:

  • Prevent infinite serialization loops caused by UIDocument rootVisualElement circular references in the generic component serializer.
  • Allow GameObject modification operations that set setActive=true to correctly find and activate inactive GameObjects.

Tests:

  • Add UIDocumentSerializationTests covering various UIDocument configurations to validate safe, finite serialization and expected output structure.

Summary by CodeRabbit

  • Bug Fixes

    • Resolved serialization hangs when processing UIDocument components caused by circular references in their visual hierarchy.
  • Improvements

    • Enhanced GameObject activation to properly locate and activate inactive GameObjects during editor operations.
  • Tests

    • Added comprehensive test suite for UIDocument serialization, covering multiple asset configurations and edge cases.

✏️ Tip: You can customize this high-level summary in your review settings.

When trying to activate an inactive GameObject via manage_gameobject modify with setActive=true, the lookup would fail because inactive objects were not included in the search by default.

Now automatically sets searchInactive=true when setActive=true is specified, allowing inactive objects to be found and activated.
…inite loops (CoplayDev#585)

UIDocument.rootVisualElement contains circular parent/child references that
can cause infinite serialization loops. This adds special handling similar to
Transform and Camera components.

The fix:
- Safely serializes panelSettings, visualTreeAsset, sortingOrder, enabled, parentUI
- Explicitly skips rootVisualElement to prevent circular reference issues
- Includes a note explaining why rootVisualElement is skipped

Tested on Unity 2021.3 and Unity 6.3.
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 19, 2026

Reviewer's Guide

Adds explicit UIDocument-specific serialization in GameObjectSerializer to avoid infinite loops from rootVisualElement’s circular VisualElement hierarchy, updates GameObjectModify to allow finding inactive GameObjects when activating them, and introduces focused EditMode tests to validate UIDocument serialization behavior and the regression fix for issue #585.

Sequence diagram for UIDocument-specific serialization in GetComponentData

sequenceDiagram
    participant Caller
    participant GameObjectSerializer
    participant UIDocument
    participant AssetDatabase
    participant McpLog

    Caller->>GameObjectSerializer: GetComponentData(UIDocumentComponent, includeNonPublicSerializedFields)
    activate GameObjectSerializer
    GameObjectSerializer->>UIDocument: inspect GetType()
    GameObjectSerializer-->>GameObjectSerializer: componentType.FullName == UnityEngine.UIElements.UIDocument
    alt is_UIDocument
        GameObjectSerializer-->>GameObjectSerializer: create uiDocProperties dictionary
        GameObjectSerializer-->>UIDocument: get panelSettings property via reflection
        alt panelSettings_not_null
            GameObjectSerializer->>AssetDatabase: GetAssetPath(panelSettings)
            AssetDatabase-->>GameObjectSerializer: assetPath
            GameObjectSerializer-->>GameObjectSerializer: add panelSettings metadata
        else panelSettings_null
            GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[panelSettings] = null
        end

        GameObjectSerializer-->>UIDocument: get visualTreeAsset property via reflection
        alt visualTreeAsset_not_null
            GameObjectSerializer->>AssetDatabase: GetAssetPath(visualTreeAsset)
            AssetDatabase-->>GameObjectSerializer: assetPath
            GameObjectSerializer-->>GameObjectSerializer: add visualTreeAsset metadata
        else visualTreeAsset_null
            GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[visualTreeAsset] = null
        end

        GameObjectSerializer-->>UIDocument: get sortingOrder via reflection
        GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[sortingOrder] = value

        GameObjectSerializer-->>UIDocument: get enabled via reflection
        GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[enabled] = value

        GameObjectSerializer-->>UIDocument: get parentUI via reflection
        alt parentUI_not_null
            GameObjectSerializer-->>GameObjectSerializer: add parentUI metadata
        else parentUI_null
            GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[parentUI] = null
        end

        GameObjectSerializer-->>GameObjectSerializer: skip rootVisualElement
        GameObjectSerializer-->>GameObjectSerializer: uiDocProperties[_note] = explanation

        GameObjectSerializer-->>Caller: return UIDocument specific dictionary
    else reflection_error
        GameObjectSerializer->>McpLog: Warn(errorMessage)
        GameObjectSerializer-->>Caller: return minimal UIDocument dictionary
    end
    deactivate GameObjectSerializer
Loading

Sequence diagram for GameObjectModify.Handle with setActive and inactive search

sequenceDiagram
    participant Tool
    participant GameObjectModify
    participant ManageGameObjectCommon
    participant GameObject

    Tool->>GameObjectModify: Handle(paramsObject, targetToken, searchMethod)
    activate GameObjectModify
    GameObjectModify-->>GameObjectModify: read paramsObject[setActive]
    alt setActive_true
        GameObjectModify-->>GameObjectModify: create findParams with searchInactive = true
    else setActive_not_true
        GameObjectModify-->>GameObjectModify: findParams = null
    end

    GameObjectModify->>ManageGameObjectCommon: FindObjectInternal(targetToken, searchMethod, findParams)
    activate ManageGameObjectCommon
    ManageGameObjectCommon-->>GameObjectModify: targetGo
    deactivate ManageGameObjectCommon

    alt targetGo_null
        GameObjectModify-->>Tool: ErrorResponse(Target GameObject not found)
    else targetGo_found
        GameObjectModify-->>GameObjectModify: proceed with modification (including setActive)
        GameObjectModify-->>Tool: success response
    end
    deactivate GameObjectModify
Loading

Class diagram for UIDocument serialization and GameObject modification

classDiagram
    class GameObjectSerializer {
        +static object GetComponentData(Component c, bool includeNonPublicSerializedFields)
    }

    class UIDocument {
        +Object panelSettings
        +Object visualTreeAsset
        +int sortingOrder
        +bool enabled
        +Object parentUI
        +VisualElement rootVisualElement
    }

    class GameObjectModify {
        +static object Handle(JObject paramsObject, JToken targetToken, string searchMethod)
    }

    class ManageGameObjectCommon {
        +static GameObject FindObjectInternal(JToken targetToken, string searchMethod, JObject findParams)
    }

    class GameObject

    class VisualElement

    class AssetDatabase {
        +static string GetAssetPath(Object asset)
    }

    class McpLog {
        +static void Warn(string message)
    }

    GameObjectSerializer ..> UIDocument : special_case_serialization
    GameObjectSerializer ..> AssetDatabase : uses
    GameObjectSerializer ..> McpLog : logs_warnings
    UIDocument o-- VisualElement : rootVisualElement

    GameObjectModify ..> ManageGameObjectCommon : uses
    GameObjectModify ..> GameObject : modifies
Loading

File-Level Changes

Change Details Files
Add special-case serialization logic for UIDocument components to avoid infinite loops and control which properties are emitted.
  • Detect UIDocument components by full type name in GetComponentData and short‑circuit the generic reflection path.
  • Manually read panelSettings, visualTreeAsset, sortingOrder, enabled, and parentUI properties via reflection, handling nulls safely.
  • Represent referenced assets (panelSettings, visualTreeAsset, parentUI) as small dictionaries including name, instanceID, and assetPath where applicable.
  • Explicitly skip rootVisualElement and record a note explaining why to prevent circular parent/child traversal.
  • Wrap UIDocument property access in a try/catch and log a warning if reflection fails, then return a compact component dictionary with metadata (typeName, instanceID, name, tag, gameObjectInstanceID).
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs
Allow GameObject modification operations that set setActive=true to locate inactive GameObjects by searching inactive objects.
  • When setActive=true is present in parameters, construct a findParams JObject with searchInactive=true.
  • Pass findParams through to ManageGameObjectCommon.FindObjectInternal so lookups can include inactive objects.
  • Fallback to existing behavior for other modification operations with no setActive flag.
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs
Introduce EditMode tests covering UIDocument serialization scenarios and guarding against the previous infinite loop behavior.
  • Create a temporary UXML asset on disk and associated VisualTreeAsset in SetUp, and clean them up in TearDown using AssetDatabase.
  • Add tests that serialize UIDocument with both panelSettings and visualTreeAsset assigned under a Timeout to catch hangs.
  • Add tests that verify UIDocument serialization returns expected structure and typeName.
  • Add separate success-path tests for UIDocument with no assets, only panelSettings, and only visualTreeAsset to ensure robustness across configurations.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs

Assessment against linked issues

Issue Objective Addressed Explanation
#585 Add special-case handling for UnityEngine.UIElements.UIDocument in GameObjectSerializer.GetComponentData so that UIDocument components are serialized without traversing rootVisualElement (avoiding circular references and infinite loops) while still capturing key properties.
#585 Ensure UIDocument serialization behaves correctly across different configurations (both assets assigned, only panelSettings, only visualTreeAsset, or neither) without hanging or throwing errors.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 19, 2026

Warning

Rate limit exceeded

@dsarno has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between d899c65 and e3ceb2d.

📒 Files selected for processing (2)
  • MCPForUnity/Editor/Helpers/GameObjectSerializer.cs
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

The PR addresses an infinite loop issue when serializing UIDocument components by implementing special-case handling that safely extracts properties while skipping circular VisualElement references, enables activation of inactive GameObjects during modification operations, and provides comprehensive test coverage validating UIDocument serialization behavior.

Changes

Cohort / File(s) Summary
UIDocument Serialization Fix
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs
Added special-case branch in GetComponentData to safely handle UIDocument components. Extracts panelSettings, visualTreeAsset, sortingOrder, enabled state, and parentUI while intentionally skipping rootVisualElement to prevent infinite loops from circular references. Includes try/catch logging and returns structured component data.
Inactive GameObject Activation
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs
Modified setActive logic to pass searchInactive=true to FindObjectInternal when activation is requested, allowing lookup of inactive GameObjects. Changes behavior only when setActive is true; otherwise preserves original lookup logic.
UIDocument Serialization Tests
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs
New test suite with 5 NUnit test methods validating UIDocument serialization under various configurations: both assets assigned, missing assets, only panelSettings, and only visualTreeAsset. Includes timeout enforcement to catch hanging behavior, temporary UXML asset creation/cleanup, and inconclusive skip logic for asset creation failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Fix material mesh instantiation warnings #331: Also modifies GetComponentData implementation in GameObjectSerializer.cs to extend component serialization handling with additional logic paths.
  • Autoformat code #297: Refactors and extends the GetComponentData method in GameObjectSerializer.cs, overlapping with the same core serialization infrastructure.

Poem

🐰 A Rabbit's Tale of Loops Undone

The UIDocument once spun circles round,
With VisualElements in loops profound,
But now we skip the rootVisualElement's embrace,
And inactive GameObjects find their place,
With tests to prove our serialization sound! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding special handling for UIDocument serialization to fix infinite loops, which is directly reflected in the changeset.
Linked Issues check ✅ Passed The PR fully addresses issue #585 by implementing special-case handling for UIDocument in GameObjectSerializer.GetComponentData(), safely serializing panelSettings, visualTreeAsset, sortingOrder, enabled, and parentUI while skipping rootVisualElement to prevent infinite loops.
Out of Scope Changes check ✅ Passed All changes are scoped to addressing issue #585: UIDocument serialization handler, logic to find inactive GameObjects when setting active, and comprehensive test coverage for UIDocument serialization scenarios.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • The UIDocument special-case builds its own return dictionary shape; consider refactoring to reuse the shared GetComponentData construction logic (or a helper) so future changes to the common component payload structure don’t require updates in multiple places.
  • The panelSettings/visualTreeAsset serialization blocks duplicate the same asset-to-dictionary mapping logic; extracting a small helper to serialize a UnityEngine.Object reference (name, instanceID, assetPath) would reduce repetition and make it easier to keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The UIDocument special-case builds its own return dictionary shape; consider refactoring to reuse the shared `GetComponentData` construction logic (or a helper) so future changes to the common component payload structure don’t require updates in multiple places.
- The panelSettings/visualTreeAsset serialization blocks duplicate the same asset-to-dictionary mapping logic; extracting a small helper to serialize a `UnityEngine.Object` reference (name, instanceID, assetPath) would reduce repetition and make it easier to keep behavior consistent.

## Individual Comments

### Comment 1
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs:144-152` </location>
<code_context>
+            var resultDict = result as Dictionary<string, object>;
+            Assert.IsNotNull(resultDict, "Result should be a dictionary");
+            
+            // Check for expected keys
+            Assert.IsTrue(resultDict.ContainsKey("typeName"), "Should contain typeName");
+            Assert.IsTrue(resultDict.ContainsKey("instanceID"), "Should contain instanceID");
+            
+            // Verify type name
+            Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"], 
+                "typeName should be UIDocument");
+        }
</code_context>

<issue_to_address>
**suggestion (testing):** Strengthen this test to validate the full UIDocument serialization contract, especially the `properties` object and absence of `rootVisualElement`

This currently only checks that serialization returns a dictionary and that `typeName` is correct, but the regression is specifically about which *properties* are serialized. To better lock down the UIDocument serialization behavior, consider:

- Asserting `resultDict` contains a `"properties"` key of type `Dictionary<string, object>`.
- Within `properties`, asserting presence of `panelSettings`, `visualTreeAsset`, `sortingOrder`, `enabled`, `parentUI`, and `_note`.
- Verifying `_note` contains the expected message about skipping `rootVisualElement`.
- Asserting there is **no** `rootVisualElement` key in `properties` (demonstrates the infinite-loop fix).
- Optionally checking `sortingOrder` is `42`.

These checks will directly validate the special handling and guard against regressions in the serialized properties.

```suggestion
            // Act
            var result = GameObjectSerializer.GetComponentData(uiDocument);

            // Assert
            Assert.IsNotNull(result, "Should return serialized component data");

            var resultDict = result as Dictionary<string, object>;
            Assert.IsNotNull(resultDict, "Result should be a dictionary");

            // Basic top-level contract
            Assert.IsTrue(resultDict.ContainsKey("typeName"), "Should contain typeName");
            Assert.IsTrue(resultDict.ContainsKey("instanceID"), "Should contain instanceID");
            Assert.AreEqual(
                "UnityEngine.UIElements.UIDocument",
                resultDict["typeName"],
                "typeName should be UIDocument");

            // Validate properties object
            Assert.IsTrue(resultDict.ContainsKey("properties"), "Should contain properties object");
            var properties = resultDict["properties"] as Dictionary<string, object>;
            Assert.IsNotNull(properties, "properties should be a Dictionary<string, object>");

            // Required serialized properties
            Assert.IsTrue(properties.ContainsKey("panelSettings"), "properties should contain panelSettings");
            Assert.IsTrue(properties.ContainsKey("visualTreeAsset"), "properties should contain visualTreeAsset");
            Assert.IsTrue(properties.ContainsKey("sortingOrder"), "properties should contain sortingOrder");
            Assert.IsTrue(properties.ContainsKey("enabled"), "properties should contain enabled");
            Assert.IsTrue(properties.ContainsKey("parentUI"), "properties should contain parentUI");
            Assert.IsTrue(properties.ContainsKey("_note"), "properties should contain _note");

            // _note should explain that rootVisualElement is skipped
            var note = properties["_note"] as string;
            Assert.IsNotNull(note, "_note should be a string");
            StringAssert.Contains("rootVisualElement", note, "_note should mention rootVisualElement being skipped");

            // rootVisualElement must not be serialized (infinite loop guard)
            Assert.IsFalse(
                properties.ContainsKey("rootVisualElement"),
                "properties should not contain rootVisualElement (to avoid infinite serialization loop)");

            // Optionally verify sortingOrder value
            var sortingOrder = Convert.ToInt32(properties["sortingOrder"]);
            Assert.AreEqual(42, sortingOrder, "sortingOrder should be 42 in this test setup");

```
</issue_to_address>

### Comment 2
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs:188-118` </location>
<code_context>
+        /// Test that UIDocument with only panelSettings assigned doesn't cause issues.
+        /// </summary>
+        [Test]
+        public void GetComponentData_UIDocument_WithOnlyPanelSettings_Succeeds()
+        {
+            // Arrange
+            var uiDocument = testGameObject.AddComponent<UIDocument>();
+            uiDocument.panelSettings = testPanelSettings;
+            // Don't assign visualTreeAsset
+
+            // Act
+            var result = GameObjectSerializer.GetComponentData(uiDocument);
+
+            // Assert
+            Assert.IsNotNull(result, "Should return serialized component data");
+        }
+
</code_context>

<issue_to_address>
**suggestion (testing):** For the partial-asset scenarios, assert the serialized `properties` reflect the expected null/non-null asset values

This test (and `GetComponentData_UIDocument_WithOnlyVisualTreeAsset_Succeeds`) only checks that serialization returns a non-null object. Since we want to validate handling of different asset combinations, please also inspect the serialized `properties` dictionary: in the `WithOnlyPanelSettings` test, assert `panelSettings` is non-null and `visualTreeAsset` is null, and vice versa in the `WithOnlyVisualTreeAsset` test. This will verify the payload contents, not just that serialization succeeds.

Suggested implementation:

```csharp
        /// <summary>
        /// Test that UIDocument with only panelSettings assigned doesn't cause issues.
        /// Also validates that panelSettings is serialized and visualTreeAsset is null.
        /// </summary>
        [Test]
        public void GetComponentData_UIDocument_WithOnlyPanelSettings_Succeeds()
        {
            // Arrange
            var uiDocument = testGameObject.AddComponent<UIDocument>();
            uiDocument.panelSettings = testPanelSettings;
            // Don't assign visualTreeAsset

            // Act
            var result = GameObjectSerializer.GetComponentData(uiDocument);

            // Assert
            Assert.IsNotNull(result, "Should return serialized component data");

            var resultDict = result as Dictionary<string, object>;
            Assert.IsNotNull(resultDict, "Result should be a dictionary");
            Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"]);

            Assert.IsTrue(resultDict.TryGetValue("properties", out var propertiesObj),
                "Serialized UIDocument should contain a 'properties' entry");

            var properties = propertiesObj as Dictionary<string, object>;
            Assert.IsNotNull(properties, "'properties' should be a dictionary");

            Assert.IsTrue(properties.ContainsKey("panelSettings"),
                "'properties' should contain 'panelSettings'");
            Assert.IsTrue(properties.ContainsKey("visualTreeAsset"),
                "'properties' should contain 'visualTreeAsset'");

            Assert.IsNotNull(properties["panelSettings"],
                "'panelSettings' should be non-null when only panelSettings is assigned");
            Assert.IsNull(properties["visualTreeAsset"],
                "'visualTreeAsset' should be null when it is not assigned");
        }

```

```csharp
        /// <summary>
        /// Test that UIDocument with only visualTreeAsset assigned doesn't cause issues.
        /// Also validates that visualTreeAsset is serialized and panelSettings is null.
        /// </summary>
        [Test]
        public void GetComponentData_UIDocument_WithOnlyVisualTreeAsset_Succeeds()
        {
            // Arrange
            var uiDocument = testGameObject.AddComponent<UIDocument>();
            uiDocument.visualTreeAsset = testVisualTreeAsset;
            // Don't assign panelSettings

            // Act
            var result = GameObjectSerializer.GetComponentData(uiDocument);

            // Assert
            Assert.IsNotNull(result, "Should return serialized component data");

            var resultDict = result as Dictionary<string, object>;
            Assert.IsNotNull(resultDict, "Result should be a dictionary");
            Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"]);

            Assert.IsTrue(resultDict.TryGetValue("properties", out var propertiesObj),
                "Serialized UIDocument should contain a 'properties' entry");

            var properties = propertiesObj as Dictionary<string, object>;
            Assert.IsNotNull(properties, "'properties' should be a dictionary");

            Assert.IsTrue(properties.ContainsKey("panelSettings"),
                "'properties' should contain 'panelSettings'");
            Assert.IsTrue(properties.ContainsKey("visualTreeAsset"),
                "'properties' should contain 'visualTreeAsset'");

            Assert.IsNull(properties["panelSettings"],
                "'panelSettings' should be null when it is not assigned");
            Assert.IsNotNull(properties["visualTreeAsset"],
                "'visualTreeAsset' should be non-null when only visualTreeAsset is assigned");
        }

```

These edits assume:

1. `GameObjectSerializer.GetComponentData(uiDocument)` returns a `Dictionary<string, object>` with:
   - `typeName` set to `"UnityEngine.UIElements.UIDocument"`.
   - A `properties` entry whose value is a `Dictionary<string, object>` containing `panelSettings` and `visualTreeAsset` keys.

If the actual shape of the serialized data differs (e.g., different key names, nested structures, or `properties` being a different type), adjust:
- The `"properties"` key and the cast to `Dictionary<string, object>`.
- The `"panelSettings"` / `"visualTreeAsset"` property names.
- The null/non-null checks to match however unassigned assets are represented (e.g., missing key vs. explicit null).
</issue_to_address>

### Comment 3
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs:11-20` </location>
<code_context>
+                "typeName should be UIDocument");
+        }
+
+        /// <summary>
+        /// Test that UIDocument WITHOUT assets assigned doesn't cause issues.
+        /// This is a baseline test - the bug only occurs with both assets assigned.
+        /// </summary>
+        [Test]
+        public void GetComponentData_UIDocument_WithoutAssets_Succeeds()
+        {
+            // Arrange - Add UIDocument component WITHOUT assets
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding a test that covers `parentUI` serialization for UIDocument hierarchies

The `parentUI` property has custom handling but no tests currently cover it. To exercise the UIDocument-specific path, add a test that:

- Creates two GameObjects, each with a UIDocument.
- Sets one as the `parentUI` of the other.
- Serializes the child UIDocument and asserts `properties["parentUI"]` matches the expected `name` and `instanceID`, and is null when no parent is set.

This ensures the parent reference is serialized correctly and guards against circular-reference regressions.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +11 to +20
/// <summary>
/// Tests for UIDocument component serialization.
/// Reproduces issue #585: UIDocument component causes infinite loop when serializing components
/// due to circular parent/child references in rootVisualElement.
/// </summary>
public class UIDocumentSerializationTests
{
private GameObject testGameObject;
private PanelSettings testPanelSettings;
private VisualTreeAsset testVisualTreeAsset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding a test that covers parentUI serialization for UIDocument hierarchies

The parentUI property has custom handling but no tests currently cover it. To exercise the UIDocument-specific path, add a test that:

  • Creates two GameObjects, each with a UIDocument.
  • Sets one as the parentUI of the other.
  • Serializes the child UIDocument and asserts properties["parentUI"] matches the expected name and instanceID, and is null when no parent is set.

This ensures the parent reference is serialized correctly and guards against circular-reference regressions.

… structure

- Add SerializeAssetReference() helper for consistent asset reference serialization
- UIDocument now uses same return structure as Camera (typeName, instanceID, properties)
- Reduces code duplication in special-case handlers
- Enhanced test coverage to verify structure matches Camera pattern
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Helpers/GameObjectSerializer.cs`:
- Around line 223-226: The check for UIDocument currently compares
componentType.FullName to "UnityEngine.UIElements.UIDocument" which misses
subclasses; update the logic in GameObjectSerializer where
componentType.FullName is compared (the UIDocument handling block) to walk the
componentType's base-type chain by name (e.g., loop through Type.BaseType and
compare FullName) so any derived type whose ancestor is
"UnityEngine.UIElements.UIDocument" triggers the same special-case handling and
avoids serializing the rootVisualElement via the generic reflection path.
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs (1)

147-160: Strengthen assertions on the serialized UIDocument payload.

This test currently validates only top-level keys/type. Consider asserting the properties payload (including the skip note) and the absence of rootVisualElement to lock in the fix.

✅ Example assertions to add
             // Check for expected keys
             Assert.IsTrue(resultDict.ContainsKey("typeName"), "Should contain typeName");
             Assert.IsTrue(resultDict.ContainsKey("instanceID"), "Should contain instanceID");
+            Assert.IsTrue(resultDict.ContainsKey("properties"), "Should contain properties");
+
+            var props = resultDict["properties"] as Dictionary<string, object>;
+            Assert.IsNotNull(props, "properties should be a dictionary");
+            Assert.IsTrue(props.ContainsKey("panelSettings"), "Should include panelSettings");
+            Assert.IsTrue(props.ContainsKey("visualTreeAsset"), "Should include visualTreeAsset");
+            Assert.IsTrue(props.ContainsKey("_note"), "Should include rootVisualElement skip note");
+            Assert.IsFalse(props.ContainsKey("rootVisualElement"), "Should not include rootVisualElement");

…VisualElement

Address code review feedback:
- Add IsOrDerivedFrom() helper to detect UIDocument and any subclasses by walking
  the base-type chain, ensuring derived types also get special-case handling
- Add negative assertion verifying rootVisualElement is NOT in serialized output
@dsarno dsarno merged commit aaf6308 into CoplayDev:main Jan 19, 2026
2 checks passed
vbucc added a commit to Studio-Pronto/unity-mcp that referenced this pull request Jan 19, 2026
Upstream changes (v9.0.7 → v9.0.8):
- fix: UIDocument serialization to prevent infinite loops (CoplayDev#586)
- fix: Filter isCompiling false positives in Play mode (CoplayDev#582)
- fix: search inactive objects when setActive=true (CoplayDev#581)
- fix: Add Prefab Stage support for GameObject lookup (CoplayDev#573)
- fix: Prevent infinite compilation loop in Unity 6 (CoplayDev#559)
- fix: parse and validate read_console types (CoplayDev#565)
- fix: Local HTTP server UI check (CoplayDev#556)
- fix: Claude Code HTTP Remote UV path override detection
- fix: ULF detection in Claude licensing (CoplayDev#569)
- chore: Replace asmdef GUID references (CoplayDev#564)
- docs: Streamline README for faster onboarding (CoplayDev#583)
- Many new client configurators (VSCode, Cursor, Windsurf, etc.)

Fork enhancements preserved:
- "find" instruction handler in UnityTypeConverters.cs
- MarkSceneOrPrefabDirty() helper for proper Prefab Stage support
- IsInPrefabStage() and GetPrefabStageRoot() helpers
- #main URL reference (no version tags)
- TestProjects excluded

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@dsarno dsarno deleted the fix/585-uidocument-infinite-loop branch January 22, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UIDocument component causes infinite loop when serializing components

1 participant