fix: prefab stage dirty flag, root rename, test fix, and prefab resources #627
Conversation
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
Contributor
Reviewer's GuideMarks prefab stage scenes dirty when modifying components, adds automatic prefab asset renaming when the prefab root is renamed, fixes a nested prefab hierarchy test’s expectations and cleanup order, and introduces new MCP prefab resources for querying prefab info and hierarchies from Unity. Sequence diagram for prefab info MCP resource requestsequenceDiagram
actor Client
participant MCPServer
participant PrefabResource as PrefabResourceService
participant UnityInstance
participant UnityTransport as UnityTransportLayer
participant UnityEditor
Client->>MCPServer: HTTP request for mcpforunity://prefab/{encoded_path}
MCPServer->>PrefabResource: get_prefab_info(ctx, encoded_path)
PrefabResource->>PrefabResource: _decode_prefab_path(encoded_path)
PrefabResource->>UnityInstance: get_unity_instance_from_context(ctx)
PrefabResource->>UnityTransport: send_with_unity_instance(async_send_command_with_retry unity_instance manage_prefabs { action: get_info, prefabPath: decoded_path })
UnityTransport->>UnityEditor: manage_prefabs get_info(prefabPath)
UnityEditor-->>UnityTransport: Prefab info response dict
UnityTransport-->>PrefabResource: response
PrefabResource->>PrefabResource: _normalize_response(response)
PrefabResource-->>MCPServer: PrefabInfoResponse
MCPServer-->>Client: Serialized MCPResponse
Class diagram for new prefab MCP resource modelsclassDiagram
class MCPResponse {
+bool success
+str message
+Any data
}
class PrefabInfoData {
+str assetPath
+str guid
+str prefabType
+str rootObjectName
+list~str~ rootComponentTypes
+int childCount
+bool isVariant
+str parentPrefab
}
class PrefabInfoResponse {
+PrefabInfoData data
}
class PrefabHierarchyItem {
+str name
+int instanceId
+str path
+bool activeSelf
+int childCount
+list~str~ componentTypes
+dict~str, Any~ prefab
}
class PrefabHierarchyData {
+str prefabPath
+int total
+list~PrefabHierarchyItem~ items
}
class PrefabHierarchyResponse {
+PrefabHierarchyData data
}
MCPResponse <|-- PrefabInfoResponse
MCPResponse <|-- PrefabHierarchyResponse
PrefabInfoResponse o--> PrefabInfoData
PrefabHierarchyResponse o--> PrefabHierarchyData
PrefabHierarchyData o--> PrefabHierarchyItem
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Note Other AI code review bot(s) detectedCodeRabbit 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. 📝 WalkthroughWalkthroughAdds prefab-root asset rename handling when renaming open-prefab roots, ensures prefab-stage scenes are marked dirty after component edits, and introduces server read-only endpoints to fetch prefab info and hierarchy by encoded path. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/LLM
participant Server as MCP Server
participant Context as Server Context
participant Transport as Transport Layer
participant Unity as Unity Editor
Client->>Server: GET /prefab/{encoded_path} or /prefab/{encoded_path}/hierarchy
Server->>Server: Decode URL-encoded path
Server->>Context: Resolve Unity instance from context
Server->>Transport: Send action ("get_info"/"get_hierarchy") with retry
Transport->>Unity: Request prefab info/hierarchy
Unity-->>Transport: Return prefab data
Transport-->>Server: Deliver response
Server->>Server: Normalize to PrefabInfo/Hierarchy model
Server-->>Client: Return JSON response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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. 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 1 issue, and left some high level feedback:
- The
ManageComponentschanges repeat the samePrefabStageUtility.GetCurrentPrefabStage/EditorSceneManager.MarkSceneDirtyblock three times; consider extracting this into a small helper method (e.g.,MarkOwningSceneDirty(GameObject targetGo)) to avoid duplication and keep future changes to this logic in one place. - In
prefab.pythe specific response models (PrefabInfoResponse,PrefabHierarchyResponse) are defined but never actually used in the resource handlers or_normalize_response; either wire these into the returned types for stronger typing/validation or remove them to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ManageComponents` changes repeat the same `PrefabStageUtility.GetCurrentPrefabStage` / `EditorSceneManager.MarkSceneDirty` block three times; consider extracting this into a small helper method (e.g., `MarkOwningSceneDirty(GameObject targetGo)`) to avoid duplication and keep future changes to this logic in one place.
- In `prefab.py` the specific response models (`PrefabInfoResponse`, `PrefabHierarchyResponse`) are defined but never actually used in the resource handlers or `_normalize_response`; either wire these into the returned types for stronger typing/validation or remove them to avoid dead code.
## Individual Comments
### Comment 1
<location> `Server/src/services/resources/prefab.py:21-25` </location>
<code_context>
+from transport.legacy.unity_connection import async_send_command_with_retry
+
+
+def _normalize_response(response: dict | Any) -> MCPResponse:
+ """Normalize Unity transport response to MCPResponse."""
+ if isinstance(response, dict):
+ return MCPResponse(**response)
+ return response
+
+
</code_context>
<issue_to_address>
**issue:** The normalization helper may return different types, which can complicate downstream handling.
`_normalize_response` is annotated to return `MCPResponse`, but if `response` is not a `dict` it returns the original object, so callers may receive non-`MCPResponse` values. Either constrain the input to `MCPResponse | dict` and always return an `MCPResponse`, or update the return type to a union that reflects the actual behavior so downstream code can handle it correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dsarno
force-pushed
the
feature/prefab-root-rename-and-text-search
branch
from
bd812b7 to
1e951af
Compare
![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
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs`:
- Around line 40-66: The collision check blocks renames when the new filename
equals the current prefab's filename because LoadAssetAtPath returns the same
asset; update the logic in the PrefabStage rename block (symbols:
PrefabStageUtility.GetCurrentPrefabStage, prefabStageForRename,
prefabContentsRoot, targetGo, assetPath, newAssetPath) to first skip any work if
newAssetPath == assetPath, otherwise compare GUIDs via
AssetDatabase.AssetPathToGUID on assetPath and newAssetPath and treat it as a
collision only if the GUIDs differ and the new path already has an asset; only
call AssetDatabase.RenameAsset when the path actually changes and no conflicting
GUID exists, and return the existing error handling (renameError) as before.
1e951af to
ae4f931
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs`:
- Around line 444-448: The cleanup contains a duplicate deletion:
SafeDeleteAsset(parentPath) and SafeDeleteAsset(Path.Combine(TempDirectory,
"ParentPrefab.prefab").Replace('\\','/')) both delete the same ParentPrefab;
remove the redundant call so ParentPrefab is deleted only once. Specifically,
keep the deletion that uses the existing parentPath variable and delete the
SafeDeleteAsset(...) that constructs "ParentPrefab.prefab" from TempDirectory
(or vice versa), ensuring the remaining order still deletes NestedContainer,
then the parent (parentPath), then ChildPrefab via SafeDeleteAsset.
♻️ Duplicate comments (2)
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs (1)
40-66: False collision detection when renaming a prefab root to its current filename.The collision check at line 53 incorrectly blocks rename operations when the new name matches the current prefab asset filename because
LoadAssetAtPathreturns the same asset. Use GUID comparison to distinguish between the current asset and actual collisions, and skip the rename operation if the path hasn't changed.🛠️ Proposed fix
// Check if we're renaming the root object of an open prefab stage var prefabStageForRename = PrefabStageUtility.GetCurrentPrefabStage(); bool isRenamingPrefabRoot = prefabStageForRename != null && prefabStageForRename.prefabContentsRoot == targetGo; if (isRenamingPrefabRoot) { // Rename the prefab asset file to match the new name (avoids Unity dialog) string assetPath = prefabStageForRename.assetPath; string directory = System.IO.Path.GetDirectoryName(assetPath); string newAssetPath = System.IO.Path.Combine(directory, name + ".prefab").Replace('\\', '/'); - // Check if a file already exists at the new path - if (AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(newAssetPath) != null) - { - return new ErrorResponse($"Cannot rename prefab root to '{name}': a prefab already exists at '{newAssetPath}'."); - } - - // Rename the asset file first - string renameError = AssetDatabase.RenameAsset(assetPath, name); - if (!string.IsNullOrEmpty(renameError)) - { - return new ErrorResponse($"Failed to rename prefab asset: {renameError}"); - } - - McpLog.Info($"[GameObjectModify] Renamed prefab asset from '{assetPath}' to '{newAssetPath}'"); + // Check if a different asset already exists at the new path + string currentGuid = AssetDatabase.AssetPathToGUID(assetPath); + string targetGuid = AssetDatabase.AssetPathToGUID(newAssetPath); + if (!string.IsNullOrEmpty(targetGuid) && targetGuid != currentGuid) + { + return new ErrorResponse($"Cannot rename prefab root to '{name}': a prefab already exists at '{newAssetPath}'."); + } + + // Rename the asset file only if the path actually changes + if (newAssetPath != assetPath) + { + string renameError = AssetDatabase.RenameAsset(assetPath, name); + if (!string.IsNullOrEmpty(renameError)) + { + return new ErrorResponse($"Failed to rename prefab asset: {renameError}"); + } + + McpLog.Info($"[GameObjectModify] Renamed prefab asset from '{assetPath}' to '{newAssetPath}'"); + } }Server/src/services/resources/prefab.py (1)
21-25: Return type inconsistency in_normalize_response.The function is annotated to return
MCPResponse, but whenresponseis not adict, it returns the original object unchanged (which could be any type). This inconsistency can cause downstream type confusion.🛠️ Proposed fix
-def _normalize_response(response: dict | Any) -> MCPResponse: +def _normalize_response(response: dict | MCPResponse | Any) -> MCPResponse: """Normalize Unity transport response to MCPResponse.""" if isinstance(response, dict): return MCPResponse(**response) + if isinstance(response, MCPResponse): + return response + # Fallback: wrap unexpected types in an error response + return MCPResponse(success=False, error=f"Unexpected response type: {type(response).__name__}") - return response
🧹 Nitpick comments (1)
Server/src/services/resources/prefab.py (1)
93-111: Consider using typed response models for better API documentation.The Pydantic models (
PrefabInfoResponse,PrefabHierarchyResponse) are defined but unused, as noted by the TODO comment. While the current implementation works, using these typed responses would provide better IDE support and automatic schema validation.For now, this is acceptable since the TODO documents the intent for future improvement.
Also applies to: 143-163
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsCrudTests.cs
Show resolved
Hide resolved
…rces
- Mark prefab stage scene as dirty when manage_components adds/removes/
modifies components, ensuring save_open_stage correctly detects changes
- When renaming the root GameObject of an open prefab stage, also rename
the prefab asset file to match, preventing Unity's "file name must
match" dialog from interrupting automated workflows
- Fix ManagePrefabsCrudTests cleanup order: delete NestedContainer.prefab
before ChildPrefab.prefab to avoid missing prefab reference errors
- Remove incorrect LogAssert.Expect that expected an error that doesn't
occur in the test scenario
- Add new prefab MCP resources for inspecting prefabs:
- mcpforunity://prefab-api: Documentation for prefab resources
- mcpforunity://prefab/{path}: Get prefab asset info
- mcpforunity://prefab/{path}/hierarchy: Get full prefab hierarchy
Addresses #97 (Prefab Editor Inspection & Modification Support)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dsarno
force-pushed
the
feature/prefab-root-rename-and-text-search
branch
from
ae4f931 to
c896abd
Compare
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
Follow-up to #611 (Prefab Feature Updates). Addresses remaining issues from #97 (Prefab Editor Inspection & Modification Support).
Changes
Mark prefab stage scene as dirty in ManageComponents: When
manage_componentsadds, removes, or modifies components on a GameObject in an open prefab stage, the prefab stage scene is now marked as dirty. This ensuressave_open_stagecorrectly detects unsaved changes.Prefab root rename without dialog: When renaming the root GameObject of an open prefab stage via
manage_gameobject, the prefab asset file is now automatically renamed to match. This prevents Unity dialog from interrupting automated workflows.Test fix: Fixed
GetHierarchy_IncludesNestingInfo_ForNestedPrefabstest:LogAssert.Expectfor an error that does not occurNew prefab MCP resources (per @msanatan suggestion):
mcpforunity://prefab-api- Documentation for prefab resourcesmcpforunity://prefab/{encoded_path}- Get prefab asset info by URL-encoded pathmcpforunity://prefab/{encoded_path}/hierarchy- Get full prefab hierarchy with nested prefab infoTest plan
Generated with Claude Code
Summary by Sourcery
Ensure prefab modifications in Unity are properly tracked and expose new read-only prefab resource endpoints.
New Features:
Bug Fixes:
Tests:
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.