Added AssetDatabase.Refresh() to all command tool calls #475
Conversation
…t the tools are working with the latest version of the editor assets. This should allow the editor to recompile and update assets in the background without manual intervention.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a pre-execution AssetDatabase.Refresh() call to all command tool executions so that each command operates on the most up-to-date Unity editor assets. Sequence diagram for command execution with AssetDatabase.RefreshsequenceDiagram
actor Developer
participant CommandRegistry
participant AssetDatabase
participant CommandHandler
Developer->>CommandRegistry: ExecuteCommand(commandName, params, tcs)
CommandRegistry->>AssetDatabase: Refresh()
CommandRegistry->>CommandRegistry: GetHandlerInfo(commandName)
CommandRegistry->>CommandHandler: Invoke handler(params, tcs)
CommandHandler-->>CommandRegistry: result or completes tcs
CommandRegistry-->>Developer: result (sync) or null (async)
Developer->>CommandRegistry: InvokeCommandAsync(commandName, params)
CommandRegistry->>AssetDatabase: Refresh()
CommandRegistry->>CommandRegistry: GetHandlerInfo(commandName)
CommandRegistry->>CommandHandler: Invoke handler(params)
CommandHandler-->>CommandRegistry: Task<object>
CommandRegistry-->>Developer: Task<object>
Class diagram for CommandRegistry with AssetDatabase.Refresh integrationclassDiagram
class CommandRegistry {
+static Func~JObject, object~ GetHandler(string commandName)
+static object ExecuteCommand(string commandName, JObject params, TaskCompletionSource~string~ tcs)
+static Task~object~ InvokeCommandAsync(string commandName, JObject params)
}
class AssetDatabase {
+static void Refresh()
}
CommandRegistry ..> AssetDatabase : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdded a using UnityEditor directive to CommandRegistry. Modified ExecuteCommand and InvokeCommandAsync methods to call AssetDatabase.Refresh before executing or invoking commands respectively. No changes to control flow logic. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Calling AssetDatabase.Refresh() on every command execution may introduce noticeable overhead; consider limiting refresh to only commands that actually change assets or adding a flag/parameter so callers can opt-in when needed.
- AssetDatabase.Refresh() is duplicated in both ExecuteCommand and InvokeCommandAsync; consider extracting a small helper or centralizing the pre-execution logic so behavior stays consistent and easier to adjust later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Calling AssetDatabase.Refresh() on every command execution may introduce noticeable overhead; consider limiting refresh to only commands that actually change assets or adding a flag/parameter so callers can opt-in when needed.
- AssetDatabase.Refresh() is duplicated in both ExecuteCommand and InvokeCommandAsync; consider extracting a small helper or centralizing the pre-execution logic so behavior stays consistent and easier to adjust later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
It is worth noting that in my limited testing, tools like |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Tools/CommandRegistry.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Tools/CommandRegistry.cs
📚 Learning: 2025-09-03T16:00:55.839Z
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.
Applied to files:
MCPForUnity/Editor/Tools/CommandRegistry.cs
📚 Learning: 2025-10-13T13:27:23.040Z
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.
Applied to files:
MCPForUnity/Editor/Tools/CommandRegistry.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Tools/CommandRegistry.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Tools/CommandRegistry.cs (2)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
Refresh(78-118)MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (1)
Refresh(49-69)
⏰ 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/Editor/Tools/CommandRegistry.cs (1)
11-11: LGTM: Required directive for AssetDatabase access.The
UnityEditornamespace addition is necessary and correctly placed to support theAssetDatabase.Refresh()calls.
| /// <returns>The result for synchronous commands, or null for async commands (TCS will be completed later)</returns> | ||
| public static object ExecuteCommand(string commandName, JObject @params, TaskCompletionSource<string> tcs) | ||
| { | ||
| AssetDatabase.Refresh(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find MCPForUnity -name "CommandRegistry.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 106
🏁 Script executed:
git ls-files "MCPForUnity/Editor/Tools/CommandRegistry.cs"Repository: CoplayDev/unity-mcp
Length of output: 106
🏁 Script executed:
fd -t f "CommandRegistry.cs" --max-results 5Repository: CoplayDev/unity-mcp
Length of output: 155
🏁 Script executed:
cat -n "MCPForUnity/Editor/Tools/CommandRegistry.cs"Repository: CoplayDev/unity-mcp
Length of output: 19060
🏁 Script executed:
# Search for where ExecuteCommand and InvokeCommandAsync are called
rg -n "ExecuteCommand|InvokeCommandAsync" --type cs -g '!CommandRegistry.cs' | head -30Repository: CoplayDev/unity-mcp
Length of output: 1061
🏁 Script executed:
# Find tool and resource implementations to see what they do
fd -t f -e "cs" --path "*Editor*" | xargs grep -l "\[McpForUnityTool\]\|\[McpForUnityResource\]" 2>/dev/null | head -10Repository: CoplayDev/unity-mcp
Length of output: 314
🏁 Script executed:
# Find tool and resource implementations
find MCPForUnity/Editor -type f -name "*.cs" -exec grep -l "\[McpForUnityTool\]\|\[McpForUnityResource\]" {} \;Repository: CoplayDev/unity-mcp
Length of output: 274
🏁 Script executed:
# Look for actual tool implementations
find MCPForUnity/Editor/Tools -type f -name "*.cs" ! -name "CommandRegistry.cs" | head -20Repository: CoplayDev/unity-mcp
Length of output: 655
🏁 Script executed:
# Check a few tool implementations to see what they do
echo "=== ExecuteMenuItem.cs ===" && head -60 MCPForUnity/Editor/Tools/ExecuteMenuItem.cs
echo -e "\n=== ReadConsole.cs ===" && head -60 MCPForUnity/Editor/Tools/ReadConsole.cs
echo -e "\n=== ManageAsset.cs ===" && head -60 MCPForUnity/Editor/Tools/ManageAsset.csRepository: CoplayDev/unity-mcp
Length of output: 6648
🏁 Script executed:
# Check BatchExecute to see command execution patterns
cat -n MCPForUnity/Editor/Tools/BatchExecute.cs | head -110Repository: CoplayDev/unity-mcp
Length of output: 4882
🏁 Script executed:
# Check if there are any guards around the Refresh calls - search for conditions before the refresh
rg -B5 "AssetDatabase.Refresh" MCPForUnity/Editor/Tools/CommandRegistry.csRepository: CoplayDev/unity-mcp
Length of output: 842
Reconsider unconditional AssetDatabase.Refresh() for performance reasons.
AssetDatabase.Refresh() is a blocking, expensive operation that scans the entire project filesystem and triggers asset garbage collection. Calling it before every command execution creates significant performance issues, especially for:
- High-frequency command usage or rapid sequential command invocations
- Batch operations (up to 25 commands per batch would trigger 25 consecutive refreshes)
- Commands that don't modify assets (e.g.,
ExecuteMenuItem,ReadConsole)
Consider these alternatives:
- Opt-in approach: Add a property to tool/resource attributes (e.g.,
RequiresAssetRefresh = true) so only commands that modify assets trigger a refresh - Targeted refresh: Pass specific asset paths to
AssetDatabase.Refresh(path)when known - Command-level control: Let individual commands call refresh internally when needed
- Post-modification refresh: Only refresh after commands that return modification indicators
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/CommandRegistry.cs around line 236, remove the
unconditional call to AssetDatabase.Refresh() and implement a selective refresh
strategy: add a boolean flag/property (e.g., RequiresAssetRefresh) on
command/tool attributes or command descriptors so only commands that modify
assets request a refresh; support passing specific asset paths for targeted
refresh when available; allow commands to perform their own refresh internally
when they know they changed assets; and ensure batch execution aggregates
refresh requests to run a single Refresh (or targeted Refresh with collected
paths) after the batch completes to avoid repeated global refreshes.
| /// <param name="params">Parameters to pass to the command (optional).</param> | ||
| public static Task<object> InvokeCommandAsync(string commandName, JObject @params) | ||
| { | ||
| AssetDatabase.Refresh(); |
There was a problem hiding this comment.
Same performance concern applies, with additional batch execution impact.
This method is explicitly documented (line 255) as being "used internally for features like batch execution," making the unconditional AssetDatabase.Refresh() call particularly problematic:
- Batch operations: Multiple commands in sequence would trigger multiple expensive refreshes, even though Unity coalesces file system changes and one refresh would suffice
- Async blocking: The synchronous
AssetDatabase.Refresh()blocks the async pathway, negating the benefits of async command execution - Redundant refreshes: Consecutive refresh calls provide no additional benefit since the first call captures all pending changes
The same alternatives suggested for ExecuteCommand apply here, with additional consideration for batching scenarios where refresh should ideally happen once before or after the batch, not per command.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/CommandRegistry.cs at line 261, remove the
unconditional synchronous AssetDatabase.Refresh() call because this method is
used for batch execution and async workflows; instead make refresh behavior
controllable and non-blocking by (a) adding an optional parameter or return flag
(e.g., bool requestRefresh = false) so callers can defer refresh for batches,
(b) when immediate refresh is needed schedule it via EditorApplication.delayCall
(or similar main-thread async scheduling) rather than blocking the async path,
and/or (c) for bulk operations prefer
AssetDatabase.StartAssetEditing()/StopAssetEditing() or a single consolidated
Refresh() around the batch — implement one of these approaches so individual
command executions do not trigger redundant, blocking refreshes.
|
@strich Thanks for getting this going. But This was one of my concerns that the AI review has flagged. The asset db refresh command is not cheap or fast. So we should smarten it up to only happen when needed, not every time. I believe it's only needed when we touch code, which many of the tools don't do. |
The AI is wrong though. I have multiple massive commercial projects - 30GB+ with complex custom importers etc and calling I think it is also wrong to suggest improving the batch command call - If the batching makes successive calls to the editor and some of those calls are making changes, then yes we should be calling refresh after each. If that certainly not happening, then sure it could be removed. But let's also remember that this call is harmless when there have been no changes so AFAIK there is no downside to calling it often. |
|
Ok, assuming that's true, have you stress tested it? |
Not at all. I will try to do so with my real project over the next week. However as said earlier, the run tests tool call times out through a recompile. Would be good to get your input on that. |
|
Thanks that would be great. I didn't write the run tests tool. I believe that was @JohanHoltby -- any idea why it would timeout Johan? |
|
@dsarno no I have not written this tool. Mine is an external test tool I sell on the assetstore - MooseRunner. All this relates to my other issue 416. LLMs get all jittery when we run the long recompiles. And yes there is a lot of state changes around it that can screw us over and if user uses hotreload we get more complexity. :). But yes never set it to run always. That will be super slow. And there are parameters in Unity on how recompile should be checked and different levels. I only do the assetrefresh when recompile fail. (I have rolled back to 7.0.0 (super stable -1 install bug) as I need to develop a bunch of features as I'm on a strict deadline and the newer versions is not as stable and is missing 416 to be usable for me and i don't have the time to fix the problem. :( Happy holidays |
This to me is the real problem with this MCP - It seems really fragile to editor refresh/recompile, which is absolutely critical to working with it. It's really beyond my loose understanding of the repo to try to improve it at this stage.
Can you validate that opinion further? Under what circumstances is |
I check if recompile needs to be done if so I recompile and no errors. If it recompiles and find errors then it throws errors. So this all is a lot of state transitions. In my testing i remember that when I do asset refresh it always do a domain reload. But yes AI should not be forced to do a lot of checking. Just one command to run one method, class or assembly. - this saves context and reduce confusion for AI. |
Indeed no - It will not force a domain reload unless a script has changed. So its free so long as nothing changed. |
|
Great then! :) I use hotreload, maybe that was why?? I remember I had a lot of problems just setting it to always.... I had same argument. |
|
If I add game objects to the scene, this unnecessarily refreshes assets. I don't think this is a good approach, it should be conditional for activities that need it |
How effective do you think introducing hotreload to the current tools could bring considering the effort? Currently in Play Mode doing some CRUD operations would still trigger the scene reset which is very inconvenient. |
|
@Scriptwonder its great for later steps when AI is finetuning the tests but worthless for first iteration:). So it has it place and saves time for me. I guess about 50%. I aim to do some videos after Christmas but they need a lot of editing. Prompting and waiting 20 min and then trigger research and then 10 min later trigger some new iteration which takes another 30 min is dead boring for most part... :) |
5 participants
Per issue: #474
Added AssetDatabase.Refresh() to all command tool calls to ensure that the tools are working with the latest version of the editor assets. This should allow the editor to recompile and update assets in the background without manual intervention.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.