Skip to content

Conversation

@msanatan
Copy link
Contributor

@msanatan msanatan commented Dec 29, 2025

This PR does a few things:

  • Lazy load tools in the UI
  • Find tools by looking for the McpForUnityTool attribute via reflection
  • We detect built-in tools by checking if their namespace is MCPForUnity.Editor.Tools instead of checking the asset path, which took a while

Startup time went from 6-12 seconds to around 200-500ms.

Summary by CodeRabbit

Release Notes

  • Refactor

    • Optimized tool discovery mechanism for improved performance when loading tools.
    • Implemented lazy loading for the Tools section in the editor window, reducing initial load time.
    • Simplified built-in tool detection logic.
  • Chores

    • Cleaned up unused asset metadata files.

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

We lazy load tools, remove the expensive AssetPath property, and reflect for only `McpForUnityToolAttribute`, so it's much faster.

A 6 second startup is now back to 400ms. Can still be optimised but this is good
The tests automatically cleans this up, so it likely got pushed by accident
@msanatan msanatan self-assigned this Dec 29, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 29, 2025

📝 Walkthrough

Walkthrough

Changes refactor tool discovery to use TypeCache instead of assembly scanning, eliminate AssetPath property and related caching infrastructure, simplify built-in tool detection, and introduce lazy loading for the Tools UI panel to improve startup performance.

Changes

Cohort / File(s) Summary
Tool Discovery Service Interface
MCPForUnity/Editor/Services/IToolDiscoveryService.cs
Removed AssetPath property from ToolMetadata class.
Tool Discovery Service Implementation
MCPForUnity/Editor/Services/ToolDiscoveryService.cs
Refactored discovery mechanism from assembly scanning to TypeCache.GetTypesWithAttribute<McpForUnityToolAttribute>(); eliminated AssetPath resolution, summary extraction, and related cache fields (_scriptPathCache, _summaryCache); simplified IsBuiltIn logic based on namespace prefix; removed private helper methods ResolveScriptAssetPath() and ExtractSummaryDescription(); added informational logging on discovery completion.
Editor Window
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
Introduced lazy loading for Tools section via toolsLoaded flag and EnsureToolsLoaded() method; conditionally loads and refreshes Tools UI in CreateGUI(), OnDisable(), and SwitchPanel().
Meta File
TestProjects/UnityMCPTests/Assets/Temp.meta
Deleted unused metadata file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • unity-mcp#418: Inverse operation that adds ToolMetadata.AssetPath, asset path resolution, summary extraction caching, and TypeCache-based discovery—directly conflicts with and complements this PR's removals.

Suggested reviewers

  • dsarno

Poem

🐰 Hops through the code with glee,
TypeCache now sets discovery free,
AssetPath fades, caches retreat,
Lazy tools make startup sweet!
Building faster, simpler still—
What engineering delight!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Optimise so startup is fast again' directly reflects the main objective of the PR: optimizing startup performance through lazy loading and efficient tool discovery, reducing startup time from 6-12 seconds to ~200-500ms.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f080f5 and 20ea23a.

📒 Files selected for processing (4)
  • MCPForUnity/Editor/Services/IToolDiscoveryService.cs
  • MCPForUnity/Editor/Services/ToolDiscoveryService.cs
  • MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
  • TestProjects/UnityMCPTests/Assets/Temp.meta
💤 Files with no reviewable changes (2)
  • TestProjects/UnityMCPTests/Assets/Temp.meta
  • MCPForUnity/Editor/Services/IToolDiscoveryService.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.

Applied to files:

  • MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
  • MCPForUnity/Editor/Services/ToolDiscoveryService.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/Windows/MCPForUnityEditorWindow.cs
  • MCPForUnity/Editor/Services/ToolDiscoveryService.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/Windows/MCPForUnityEditorWindow.cs
  • MCPForUnity/Editor/Services/ToolDiscoveryService.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/Windows/MCPForUnityEditorWindow.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
  • Refresh (78-118)
MCPForUnity/Editor/Services/ToolDiscoveryService.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
  • McpLog (7-52)
  • Warn (43-46)
🔇 Additional comments (8)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (5)

34-34: LGTM! Lazy loading flag added.

The toolsLoaded flag correctly tracks whether tools have been loaded, enabling the lazy loading optimization.


200-205: LGTM! Correct handling of active Tools tab.

This correctly handles the case where the Tools tab is already active when the window is created, ensuring tools are loaded immediately in that scenario.


217-231: LGTM! Lazy loading correctly implemented.

The method correctly:

  • Guards against redundant loading with the toolsLoaded flag
  • Includes defensive null check on toolsSection
  • Sets the flag before calling Refresh() to prevent re-entry issues

This implementation will defer the expensive DiscoverAllTools() call until the Tools panel is actually needed.


244-244: LGTM! Proper cleanup in OnDisable.

Resetting the toolsLoaded flag in OnDisable ensures correct behavior if the window is reopened.


353-356: LGTM! Tools loaded when panel is shown.

Calling EnsureToolsLoaded() when switching to the Tools panel ensures tools are discovered before they're displayed, while avoiding the cost during initial window creation.

MCPForUnity/Editor/Services/ToolDiscoveryService.cs (3)

26-54: LGTM! TypeCache significantly improves performance.

The switch from manual assembly scanning to TypeCache.GetTypesWithAttribute<McpForUnityToolAttribute>() is excellent. TypeCache is Unity's built-in optimized type lookup system that maintains an index automatically, making this discovery much faster than the previous approach.

The error handling is appropriate, with warnings logged for attribute read failures and null checks ensuring robustness.


135-137: LGTM! AssetPath removal simplifies the code.

Removing AssetPath resolution eliminates expensive file system operations during tool discovery, contributing to the performance improvements.


262-265: LGTM! Namespace check is much faster than asset path inspection.

Detecting built-in tools via namespace check (type.Namespace.StartsWith("MCPForUnity.Editor.Tools")) eliminates expensive asset path resolution and file system I/O. The use of StringComparison.Ordinal is appropriate for performance-sensitive namespace matching.


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.

@dsarno
Copy link
Collaborator

dsarno commented Dec 29, 2025

👏👏👏

@msanatan
Copy link
Contributor Author

msanatan commented Dec 29, 2025

Did some testing, I think users will appreciate this one!

@msanatan msanatan merged commit 1a7f4bb into CoplayDev:main Dec 29, 2025
1 check passed
@msanatan msanatan deleted the faster-startup branch December 29, 2025 22:39
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.

2 participants