fix: Multi-session UI improvements and HTTP instance recognition #517
Conversation
- Separate server and session lifecycle in HTTP Local mode - Show 'Start Server' / 'Stop Server' button for server control - Show 'Start Session' / 'End Session' button when server is running - No auto-join on server start (requires manual session start) - Show instance name instead of port in session status (e.g. 'Session Active (ramble)') - Use native project_hash for HTTP instance recognition instead of computed SHA256 - Fix test expectations for manage_asset JSON parsing error messages
Reviewer's GuideSeparates HTTP server lifecycle from session lifecycle in the Unity editor UI, improves multi-instance identification, switches HTTP instance recognition to use Unity’s native project_hash instead of a computed SHA256, and adjusts tests to match updated JSON parsing messages. Sequence diagram for HTTP Local server and session lifecycle separationsequenceDiagram
actor User
participant UnityEditor as UnityEditor_UI
participant Bridge as BridgeService
participant LocalServer as LocalHttpServer
rect rgb(230,230,250)
Note over User,LocalServer: Start Server (HTTP Local)
User->>UnityEditor: Click StartServerButton
UnityEditor->>LocalServer: StartLocalHttpServer()
alt server started
LocalServer-->>UnityEditor: true
UnityEditor->>UnityEditor: UpdateStartHttpButtonState()
UnityEditor->>UnityEditor: Show StartSessionButton
else server failed
LocalServer-->>UnityEditor: false
UnityEditor->>UnityEditor: Log warn Failed to start local HTTP server
end
end
rect rgb(230,250,230)
Note over User,Bridge: Start Session (after server running)
User->>UnityEditor: Click StartSessionButton
UnityEditor->>Bridge: StartAsync()
Bridge-->>UnityEditor: Session running
UnityEditor->>UnityEditor: UpdateConnectionStatus()
UnityEditor->>UnityEditor: Set label Session Active (instanceName)
end
rect rgb(250,230,230)
Note over User,LocalServer: Stop Server (may or may not have session)
User->>UnityEditor: Click StopServerButton
UnityEditor->>UnityEditor: serverRunning = IsHttpLocalSelected() and IsLocalHttpServerRunning()
alt serverRunning
alt session active
UnityEditor->>Bridge: StopAsync()
Bridge-->>UnityEditor: Session stopped
end
UnityEditor->>LocalServer: StopLocalHttpServer()
alt stopped
LocalServer-->>UnityEditor: true
else
LocalServer-->>UnityEditor: false
UnityEditor->>UnityEditor: Log warn Failed to stop HTTP server or no server was running
end
else server not running
UnityEditor->>UnityEditor: No-op
end
end
Class diagram for project id resolution using native project_hashclassDiagram
class CustomToolService {
+compute_project_id(project_name: str, project_path: str) str
+resolve_project_id_for_unity_instance(unity_instance: str) str
}
class ConnectionPoolTarget {
+name: str
+path: str
+hash: str
}
class ConnectionPool {
+get_target(unity_instance: str) ConnectionPoolTarget
}
CustomToolService --> ConnectionPool : uses
ConnectionPool --> ConnectionPoolTarget : returns
note for CustomToolService "DEPRECATED (compute_project_id): SHA256-based project id no longer used; Unity project_hash is used instead"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. 📝 WalkthroughWalkthroughConnection UI separates local HTTP server lifecycle from session lifecycle; server toggle now controls only the local server and orphaned sessions are cleaned up. Backend now prefers Unity-provided project hashes when resolving project IDs. Integration tests relax a JSON parsing log/message assertion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as MCP Connection UI
participant LocalServer as Local HTTP Server
participant Session as Bridge/Session Manager
participant Unity as Unity Instance
Note over UI,LocalServer: User toggles HTTP server (server-centric flow)
UI->>LocalServer: Start/Stop request
alt Starting (was stopped)
LocalServer-->>UI: Started
UI->>Session: TryAutoStartSessionAsync (optional follow-up)
else Stopping (was running)
UI->>Session: End active session if present (EndOrphanedSessionAsync)
Session-->>LocalServer: Bridge stopped / cleanup
LocalServer-->>UI: Stopped
end
Note over UI: Session toggle visibility
UI->>LocalServer: Query localServerRunning
alt localServerRunning == true
UI-->>UI: Show session toggle (HTTP Local)
else
UI-->>UI: Hide session toggle (HTTP Local)
end
Note over Session,Unity: When session active
Session->>Unity: Attach to instance (label includes instance/project folder)
Unity-->>Session: Ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 found 1 issue, and left some high level feedback:
- In
UpdateConnectionStatus, the new logic forshowSessionToggleis based onlastLocalServerRunning; consider explicitly syncing this with theserverRunningcheck used inOnHttpServerToggleClickedto avoid UI desync where the session button is visible but the local HTTP server is no longer actually running. - In
resolve_project_id_for_unity_instance,target.hashis returned directly; if there are any cases wherehashcan beNoneor empty, you may want to guard or log that situation to avoid silently returning an unusable project ID.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UpdateConnectionStatus`, the new logic for `showSessionToggle` is based on `lastLocalServerRunning`; consider explicitly syncing this with the `serverRunning` check used in `OnHttpServerToggleClicked` to avoid UI desync where the session button is visible but the local HTTP server is no longer actually running.
- In `resolve_project_id_for_unity_instance`, `target.hash` is returned directly; if there are any cases where `hash` can be `None` or empty, you may want to guard or log that situation to avoid silently returning an unusable project ID.
## Individual Comments
### Comment 1
<location> `Server/src/services/custom_tool_service.py:277-285` </location>
<code_context>
def compute_project_id(project_name: str, project_path: str) -> str:
+ """
+ DEPRECATED: Computes a SHA256-based project ID.
+ This function is no longer used as of the multi-session fix.
+ Unity instances now use their native project_hash (SHA1-based) for consistency
+ across stdio and WebSocket transports.
+ """
combined = f"{project_name}:{project_path}"
return sha256(combined.encode("utf-8")).hexdigest().upper()[:16]
</code_context>
<issue_to_address>
**suggestion:** Consider removing or isolating the deprecated `compute_project_id` helper to avoid accidental reuse.
Since project identity now relies solely on Unity’s `project_hash`, leaving this SHA256 helper in the main service module (even marked deprecated) makes it easier for future code to mistakenly reuse it. If it’s no longer needed, please remove it or move it into a migration/compat module so there’s an unambiguous single source of truth for project IDs.
Suggested implementation:
```python
```
If `compute_project_id` is still referenced elsewhere:
1. Create a dedicated compatibility/migrations module (e.g. `Server/src/services/compat/project_id_compat.py`) and move the function there.
2. Update all call sites to either:
- Use Unity’s `project_hash` directly where possible, or
- Import from the new compat module if legacy SHA256 IDs are explicitly required for migrations/backward compatibility.
3. Once all legacy use is removed, delete the compat function as well to enforce a single source of truth (`project_hash`) for project identity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def compute_project_id(project_name: str, project_path: str) -> str: | ||
| """ | ||
| DEPRECATED: Computes a SHA256-based project ID. | ||
| This function is no longer used as of the multi-session fix. | ||
| Unity instances now use their native project_hash (SHA1-based) for consistency | ||
| across stdio and WebSocket transports. | ||
| """ | ||
| combined = f"{project_name}:{project_path}" | ||
| return sha256(combined.encode("utf-8")).hexdigest().upper()[:16] |
There was a problem hiding this comment.
suggestion: Consider removing or isolating the deprecated compute_project_id helper to avoid accidental reuse.
Since project identity now relies solely on Unity’s project_hash, leaving this SHA256 helper in the main service module (even marked deprecated) makes it easier for future code to mistakenly reuse it. If it’s no longer needed, please remove it or move it into a migration/compat module so there’s an unambiguous single source of truth for project IDs.
Suggested implementation:
If compute_project_id is still referenced elsewhere:
- Create a dedicated compatibility/migrations module (e.g.
Server/src/services/compat/project_id_compat.py) and move the function there. - Update all call sites to either:
- Use Unity’s
project_hashdirectly where possible, or - Import from the new compat module if legacy SHA256 IDs are explicitly required for migrations/backward compatibility.
- Use Unity’s
- Once all legacy use is removed, delete the compat function as well to enforce a single source of truth (
project_hash) for project identity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/src/services/custom_tool_service.py (1)
269-274: Duplicate method definition:_safe_responseis defined twice.The method
_safe_responseappears at lines 262-267 and again at lines 269-274. The second definition shadows the first. While the implementations are identical, this is dead code that should be removed.🔎 Proposed fix: Remove duplicate method
def _safe_response(self, response): if isinstance(response, dict): return response if response is None: return None return {"message": str(response)} - - def _safe_response(self, response): - if isinstance(response, dict): - return response - if response is None: - return None - return {"message": str(response)}MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
607-669: Remove unreferenced methodTryAutoStartSessionAfterHttpServerAsync.The method at lines 607–669 is not called anywhere in the codebase and should be deleted. Since the new flow explicitly avoids auto-starting sessions per PR objectives, this dead code should be cleaned up.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
301-303: Consider defensive handling for edge-case paths.The instance name extraction assumes
Application.dataPathfollows the typicalProjectFolder/Assetsstructure. While this is standard for Unity projects, ifdataPathwere at a filesystem root or malformed,Path.GetDirectoryNamecould returnnull, causingPath.GetFileNameto return an empty string.This is unlikely in practice, but a fallback would improve robustness:
🔎 Proposed defensive fallback
// Show instance name (project folder name) for better identification in multi-instance scenarios. -string instanceName = System.IO.Path.GetFileName(System.IO.Path.GetDirectoryName(Application.dataPath)); +string parentDir = System.IO.Path.GetDirectoryName(Application.dataPath); +string instanceName = !string.IsNullOrEmpty(parentDir) + ? System.IO.Path.GetFileName(parentDir) + : "Unity"; connectionStatusLabel.text = $"Session Active ({instanceName})";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/custom_tool_service.pyServer/tests/integration/test_manage_asset_json_parsing.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
📚 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/Components/Connection/McpConnectionSection.cs
⏰ 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 (7)
Server/tests/integration/test_manage_asset_json_parsing.py (2)
35-37: LGTM! Test assertion simplified.The removal of the log assertion aligns with the backend change (no longer logging "coerced properties" on successful JSON parsing). The test now focuses on verifying the functional outcome rather than implementation details.
58-60: LGTM! Error message expectation updated.The updated assertion
"failed to parse properties"correctly matches the backend's error message format. Themanage_assettool surfaces this substring when properties parsing fails (line 70 of manage_asset.py), covering both JSON and literal_eval error paths.Server/src/services/custom_tool_service.py (2)
277-285: LGTM! Clear deprecation notice forcompute_project_id.The deprecation docstring clearly explains why this function is no longer used and the rationale for switching to Unity's native
project_hash. Retaining the implementation allows gradual migration and backward compatibility.
315-318: LGTM! Project ID resolution now uses Unity-provided hash.Returning
target.hashinstead of computing a SHA256-based ID aligns stdio transport behavior with WebSocket transport, ensuring consistent project identification across all transports. Thetarget.hashfield is always populated for discovered Unity instances since it's a required field inUnityInstanceInfothat gets set during instance discovery from the status file.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
276-283: LGTM! Session toggle visibility correctly separates server and session lifecycles.For HTTP Local mode, the session toggle only appears when the local server is running, requiring users to manually start/end sessions. For Stdio and HTTP Remote, the toggle is always visible. This aligns with the multi-instance scenario requirements.
450-459: LGTM! Server button state logic correctly reflects server-only lifecycle.The button now uses
localServerRunningto determine its state, independent of session status. Theserver-runningclass is applied based solely onlocalServerRunning, and the enabled state properly accounts for in-progress operations.
487-511: LGTM! Server toggle correctly separates server and session lifecycles.The refactored logic:
- Stop flow: Ends the session first (if active), then stops the server.
- Start flow: Only starts the server without auto-starting a session.
This matches the PR objectives for multi-instance scenarios where users need explicit control over session lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @Server/src/services/custom_tool_service.py:
- Around line 309-313: When a stdio Unity instance is found (the variable
target) but its hash (target.hash / project_hash) is empty, the function
currently logs a warning and then continues to the HTTP/WebSocket PluginHub
resolution using the same unity_instance string, which may return a different
project ID; fix this by returning None immediately after the logger.warning so
callers receive explicit failure for that stdio instance (ensure the warning
remains and add a single return None right after it), leaving the later
PluginHub resolution code unchanged.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
472-474: Optional: Simplify redundant condition checks.Both branches of the OR expression check
!httpServerToggleInProgress, which can be factored out:🔎 Proposed simplification
startHttpServerButton.SetEnabled( - (shouldShowStop && !httpServerToggleInProgress) || - (canStartLocalServer && !httpServerToggleInProgress)); + !httpServerToggleInProgress && (shouldShowStop || canStartLocalServer));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/custom_tool_service.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/Components/Connection/McpConnectionSection.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Info(37-41)Warn(43-46)MCPForUnity/Editor/Services/ServerManagementService.cs (3)
IsLocalHttpServerRunning(538-598)StopLocalHttpServer(506-509)StartLocalHttpServer(407-501)MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
IsRunning(132-132)MCPForUnity/Editor/Services/BridgeControlService.cs (3)
Task(80-115)Task(117-128)Task(130-136)
⏰ 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 (5)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (5)
271-283: Good reliability improvement for orphaned session detection.The logic correctly identifies when an HTTP Local session is active but the underlying server is no longer running, then automatically cleans up the orphaned session. The sequencing is well-designed:
- Line 274 refreshes
lastLocalServerRunningstate- Line 278 checks preconditions to avoid false positives
- Line 281 fires async cleanup without blocking UI
- Line 282 updates local state to prevent inconsistent UI during the rest of this method
The
connectionToggleInProgressguard prevents concurrent cleanup attempts.
285-293: LGTM: Session toggle visibility correctly implements lifecycle separation.The conditional display logic properly separates server lifecycle from session lifecycle in multi-instance scenarios:
- HTTP Local: Session controls shown only when server is running (
lastLocalServerRunninggate)- Stdio/HTTP Remote: Session controls always shown
This aligns with the PR objective to require manual session management when multiple Unity instances are running.
313-326: Well-implemented defensive instance name extraction.The multi-layered fallback handling for path parsing edge cases is appropriate:
- Line 314-316: Guards against null/empty
projectDirfromGetDirectoryName- Line 317: Guards against
GetFileNamereturning empty (e.g., if path ends with separator)- Both fallbacks default to
"Unity"for graceful degradationThe session status at line 318 now displays
"Session Active ({instanceName})"which improves multi-instance identification as described in the PR objectives.
489-539: LGTM: Server-centric toggle correctly implements lifecycle separation.The reworked flow properly separates server and session management:
- Stop path (lines 505-517): Ends active session first (if any), then stops the HTTP server
- Start path (lines 518-526): Starts HTTP server only without auto-joining a session
This removes the previous auto-start behavior and aligns with the PR objective: "No auto-join on server start; sessions must be started manually."
The sequencing prevents orphaned sessions by ensuring cleanup happens before server shutdown.
622-642: Well-designed helper for orphaned session cleanup.The fire-and-forget pattern is appropriate for this cleanup scenario:
- Lines 628-629: Sets progress flags and disables button to prevent concurrent operations
- Line 630: Calls
StopAsyncto clean up the bridge/session- Lines 636-640: Finally block ensures UI state is restored regardless of outcome
- Line 634: Logs failures as warnings (not errors) since orphaned cleanup is best-effort
The method prevents the UI from showing "Session Active" when the underlying server has stopped, maintaining consistency between UI state and actual server state.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs:
- Around line 276-283: The current orphaned-session handling sets isRunning =
false immediately which can cause UI/state races because the bridge still
reports IsRunning true until EndOrphanedSessionAsync completes; instead
introduce a dedicated bool flag (e.g., orphanedSessionCleanupInProgress) and set
it when you kick off EndOrphanedSessionAsync, clear it at the end of
EndOrphanedSessionAsync, and change UpdateConnectionStatus (and any checks using
isRunning) to treat orphanedSessionCleanupInProgress as meaning "cleanup
pending" so you avoid mutating isRunning prematurely; alternatively, if you
prefer blocking semantics, await EndOrphanedSessionAsync() inline instead of
launching it fire-and-forget, but prefer the flag approach to avoid UI thread
blocking.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
435-476: Remove unusedsessionRunningvariable.Line 450 declares
sessionRunningbut it's no longer referenced in this method after the server/session lifecycle separation. This is dead code.🔎 Proposed fix
bool useHttp = transportDropdown != null && (TransportProtocol)transportDropdown.value != TransportProtocol.Stdio; if (!useHttp) { startHttpServerButton.SetEnabled(false); startHttpServerButton.tooltip = string.Empty; return; } bool httpLocalSelected = IsHttpLocalSelected(); bool canStartLocalServer = httpLocalSelected && MCPServiceLocator.Server.IsLocalUrl(); - bool sessionRunning = MCPServiceLocator.Bridge.IsRunning; bool localServerRunning = false;
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
311-318: Consider simplifying the defensive instance name extraction.The defensive null/empty checks are thorough and correct. However, the double assignment of the fallback
"Unity"is slightly redundant.🔎 Proposed simplification
- string projectDir = System.IO.Path.GetDirectoryName(Application.dataPath); - string instanceName = !string.IsNullOrEmpty(projectDir) - ? System.IO.Path.GetFileName(projectDir) - : "Unity"; - if (string.IsNullOrEmpty(instanceName)) instanceName = "Unity"; + string projectDir = System.IO.Path.GetDirectoryName(Application.dataPath); + string instanceName = string.IsNullOrEmpty(projectDir) + ? "Unity" + : System.IO.Path.GetFileName(projectDir); + if (string.IsNullOrEmpty(instanceName)) + { + instanceName = "Unity"; + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/custom_tool_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/custom_tool_service.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/Components/Connection/McpConnectionSection.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
MCPForUnity/Editor/Services/ServerManagementService.cs (2)
IsLocalHttpServerRunning(538-598)StopLocalHttpServer(506-509)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/IServerManagementService.cs (2)
IsLocalHttpServerRunning(36-36)StopLocalHttpServer(24-24)MCPForUnity/Editor/Services/Transport/TransportManager.cs (1)
IsRunning(132-132)
🔇 Additional comments (4)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4)
271-274: LGTM: Clear documentation of dual-purpose call.The comment helpfully explains that
UpdateStartHttpButtonStateboth updates the button label and refresheslastLocalServerRunningfor subsequent visibility logic.
285-293: LGTM: Session toggle visibility correctly separates lifecycle concerns.The visibility logic properly implements the requirement to show session controls only when the server is running (HTTP Local) or always (Stdio/HTTP Remote), supporting the multi-instance workflow.
488-538: LGTM: Server toggle correctly implements lifecycle separation.The reworked flow properly separates server and session lifecycles:
- Stop flow: ends the session first, then stops the server
- Start flow: launches the server without auto-starting a session
This aligns with the PR objective to require manual session start in multi-instance scenarios.
621-641: LGTM: Orphaned session cleanup properly encapsulated.The async cleanup method correctly:
- Uses the same
connectionToggleInProgressguard as the manual toggle- Disables UI during the operation
- Logs errors without throwing
- Refreshes UI state in the finally block
The fire-and-forget pattern (line 281) is acceptable here since errors are logged internally and won't surface to the caller.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Show resolved
Hide resolved
dsarno
force-pushed
the
fix-multi-session-activate
branch
2 times, most recently
from
14abf11 to
c6a67b7
Compare
- Separate server and session lifecycle in HTTP Local mode - Show 'Start Server' / 'Stop Server' button for server control - Show 'Start Session' / 'End Session' button when server is running - Auto-start session when THIS instance starts the server - Require manual session start when connecting to external server - Auto-detect and end orphaned sessions when server stops - Show instance name instead of port in session status - Use native project_hash for HTTP instance recognition - Guard against empty hash with warning log - Remove dead code (duplicate _safe_response) - Add defensive path handling for instance name extraction
dsarno
force-pushed
the
fix-multi-session-activate
branch
from
c6a67b7 to
364a414
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
545-577: Consider adding user feedback when auto-start fails.The retry logic with polling is sound, but if the session fails to start after 10 attempts (up to 10 seconds), only a warning is logged. Users who clicked "Start Server" might not realize the session didn't start, especially since the server button will show "Stop Server" but the session toggle won't reflect an active session.
💡 Suggestion: Add user notification
After line 576, consider adding a dialog to inform the user:
McpLog.Warn("Failed to auto-start session after launching the HTTP server."); + EditorUtility.DisplayDialog( + "Session Start Failed", + "The HTTP server started successfully, but the session failed to start automatically. " + + "Please use the 'Start Session' button to connect manually.", + "OK");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/custom_tool_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/custom_tool_service.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/Components/Connection/McpConnectionSection.cs
⏰ 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 (5)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (5)
285-293: LGTM! Session toggle visibility logic is clear.The conditional visibility separates server lifecycle from session lifecycle effectively. For HTTP Local, users must start the server before they can start a session, which makes sense for multi-instance scenarios.
311-318: LGTM! Instance name extraction is defensive and clear.The defensive handling of edge cases (null/empty directory names) ensures robust UI display. The fallback to "Unity" provides a sensible default for multi-instance identification.
464-472: LGTM! Server button state logic cleanly separates concerns.The button now controls only server lifecycle, with session control delegated to
connectionToggleButton. This separation aligns well with the multi-instance UI improvements described in the PR objectives.
500-530: LGTM! Server toggle flow handles lifecycle correctly.The flow properly sequences operations:
- Stop path: ends active session before stopping the server
- Start path: launches server then auto-starts the session via
TryAutoStartSessionAsyncThe distinction between user-initiated server start (auto-start session) and detecting an external server (manual session start) provides good UX for multi-instance scenarios.
660-680: LGTM! Orphaned session cleanup is properly structured.The method correctly:
- Sets
connectionToggleInProgressflag to prevent concurrent operations- Handles exceptions gracefully
- Ensures UI state is restored in the
finallyblockThis cleanup keeps the UI synchronized with the actual server state.
1 participant
Fixes #514 and #516
When second Unity instance is running
Summary by Sourcery
Adjust Unity MCP multi-session behavior and project identification to better support multiple concurrent instances.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.