feat: auto-select Unity instance by launch directory#1194
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds launch-directory-aware Unity instance auto-selection using session project paths, MCP roots, path normalization, per-session caching, and PluginHub and stdio discovery integration. Tests cover URI parsing, matching, ambiguity, caching, metadata, and transport flows. ChangesUnity Instance Auto-selection via Launch Directory Matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UnityInstanceMiddleware
participant PluginHub
participant UnityInstances
Client->>UnityInstanceMiddleware: advertise MCP roots
UnityInstanceMiddleware->>PluginHub: get_sessions()
PluginHub-->>UnityInstanceMiddleware: instance IDs and project paths
UnityInstanceMiddleware->>UnityInstanceMiddleware: normalize paths and find unique match
UnityInstanceMiddleware->>UnityInstances: set_active_instance()
UnityInstanceMiddleware-->>Client: selected Unity instance
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
87aa320 to
4e009c7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
4e009c7 to
3617b75
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 272-280: The current _client_root_dirs logic double-fetches under
concurrency; modify it to memoize the in-flight fetch per key instead of only
storing the final dirs after await: inside _client_root_dirs (use
get_session_key, _root_dirs_by_key, _lock and _fetch_client_root_dirs) acquire
the lock, check for an existing entry and return if present, and if missing
create and store a placeholder future/task for that key before releasing the
lock; then await the actual _fetch_client_root_dirs, resolve the placeholder
with the result and replace it with the real dirs; ensure exceptions clear the
placeholder so subsequent calls can retry.
- Around line 327-345: The selector currently skips candidates with missing
project_path and may still auto-select a single match; change the logic so that
if there is more than one candidate and any candidate has a missing or falsy
project_path, the function treats the set as ambiguous and returns None instead
of auto-selecting. Concretely, before the matching loop or before returning a
single match, check the candidates list for more than one entry and for any
inst_id whose project_path is falsy; if found, return None; otherwise proceed
with the existing _strip_assets/project_real/commonpath matching that populates
matches and the existing matches==1 return behavior. Ensure this uses the
existing variables candidates, project_path, matches, _strip_assets, and
launch_reals so the change is local and minimal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da8dd5e3-4aab-4020-95bf-ad14f04bf876
📒 Files selected for processing (4)
Server/src/transport/models.pyServer/src/transport/plugin_hub.pyServer/src/transport/unity_instance_middleware.pyServer/tests/integration/test_instance_autoselect_by_launch_dir.py
🚧 Files skipped from review as they are similar to previous changes (3)
- Server/src/transport/models.py
- Server/src/transport/plugin_hub.py
- Server/tests/integration/test_instance_autoselect_by_launch_dir.py
|
@coderabbitai full review |
✅ Action performedFull review finished. |
When several Unity editors are connected, auto-select gave up at ">1 instance" and forced the caller to pass unity_instance on every call. Worktrees made it worse: identical project names, only the path differs. Resolve the directories the client is working in - client-agnostic, via the package's own UNITY_MCP_PROJECT_DIR or the file:// MCP roots the client advertises (all of them) - and pick the single connected editor whose project shares a path lineage with one of them. Candidate paths are normalized to the project root (stdio reports .../Assets, HTTP the root) before matching, and the roots lookup is cached per session to keep the no-match path off the wire. Surface project_path on SessionDetails so the PluginHub path can match too. Purely additive: only the existing ">1 instance, give up" branch changes; single-instance and explicit selection are untouched, and no/ambiguous match keeps the "ask the user" behavior.
3617b75 to
3e38a06
Compare
|
@coderabbitai full review |
jordandunmire97-ai
left a comment
jordandunmire97-ai
left a comment
There was a problem hiding this comment.
Wow I love this .13 days from now I won't be here physically in this realm. Hopefully when I am back I am a bird . With that . Also why I am approving it myself. And no im not sad. Its very hard to explain
. Don't worry. Never one has . Thank you so much for what you have done .
|
Hi @Scriptwonder, when you have a moment, would you be able to take a look at this PR? Happy to adjust anything if needed. Thanks! |
…tance-by-launch-dir # Conflicts: # Server/src/transport/unity_instance_middleware.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
104-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the list of instances for better readability.
Stringifying
self.available_instancesdirectly embeds the Python list representation (e.g.,Available instances: ['RepoA@hash', 'RepoB@hash'].). Joining the elements results in a slightly cleaner, more user-friendly error string.🔨 Proposed tweak
def __init__(self, message: str | None = None, available_instances: list[str] | None = None): # Carried structurally so the transport layer can surface the ids without # parsing the message; also appended to the text for parity with the stdio # guard, which lists the ids inline. self.available_instances = available_instances or [] text = message or self._SELECTION_REQUIRED if self.available_instances: - text = f"{text} Available instances: {self.available_instances}." + text = f"{text} Available instances: {', '.join(self.available_instances)}." super().__init__(text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 104 - 113, The PluginHub exception initializer’s available-instance message uses Python list repr formatting. Update __init__ to join self.available_instances into a readable comma-separated string before interpolating it into the “Available instances” text, while preserving the existing empty-list behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 125-141: Update get_session_key to prioritize a non-empty
ctx.session_id before client_id, so MCP root caches are isolated per session.
Preserve the existing client_id, user_id, and global fallbacks when no session
ID is available.
---
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 104-113: The PluginHub exception initializer’s available-instance
message uses Python list repr formatting. Update __init__ to join
self.available_instances into a readable comma-separated string before
interpolating it into the “Available instances” text, while preserving the
existing empty-list behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd2a1b73-daf9-45ca-813b-369e02c6442d
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Server/src/transport/models.pyServer/src/transport/plugin_hub.pyServer/src/transport/unity_instance_middleware.pyServer/tests/integration/test_instance_autoselect_by_launch_dir.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/src/transport/models.py
- Server/tests/integration/test_instance_autoselect_by_launch_dir.py
|
Hi @Scriptwonder, this PR is still relevant :) |
2 participants
Description
When several Unity editors are connected the server only auto-selects one if exactly one is open. With more than one it gives up and you have to pass
unity_instanceon every call. This happens often when the Unity project is not at the repo root, or when you run several projects at once, or git worktrees of one project. Worktrees share the same project name and differ only by path, so name alone cannot tell them apart.This PR picks the editor that matches the directory the client is working in.
Type of Change
Changes Made
The whole change lives in the selection middleware. The old code gave up in the "more than one instance" branch of
_maybe_autoselect_instance. That branch now tries to resolve a single editor before giving up.unity_instance_middleware.py: in the "more than one instance" branch, select the editor whose project path shares a path lineage with the client launch directory. The single instance path and the explicitunity_instancepath are untouched. No match or an ambiguous match keeps the current "ask the user" behavior.UNITY_MCP_PROJECT_DIRwins if set, otherwise thefile://MCP roots the client advertises. The result is cached per session so the client is queried at most once.file://URI parsing (UNC hosts, percent-encoding, Windows drive letters) and project-root normalization. Over stdio Unity reports theAssetsfolder, over HTTP the project root.models.pyandplugin_hub.py:SessionDetailsgets an optionalproject_pathso the HTTP path can match too. It defaults toNone, so this is backward compatible.Server/tests/integration/test_instance_autoselect_by_launch_dir.py.Compatibility / Package Source
#beta,#main, tag, branch, orfile:): not applicable, no Unity package change. Branch is offbeta.Packages/packages-lock.json: not applicable.Testing/Screenshots/Recordings
cd Server && uv run pytest tests/ -v), full suite passes including the new file.Documentation Updates
No tools or resources changed, so the generated tool reference is not affected.
Related Issues
Relates to the multi-instance routing item under "Areas That Need Help" in CONTRIBUTING. Independent of #1121, which hardens discovery and auto-start. This PR only changes the selection middleware.
Additional Notes
roots-based and theUNITY_MCP_PROJECT_DIR-based routing pick the right editor, and an ambiguous case picks nothing.Summary by CodeRabbit