-
Notifications
You must be signed in to change notification settings - Fork 663
Minor fixes #602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor fixes #602
Conversation
Small update for CoplayDev#542
Small update for CoplayDev#542
- Rename CSS class from generic "error" to "http-local-url-error" for better specificity - Rename "invalid-url" class to "http-local-invalid-url" for clarity - Disable httpServerCommandField when URL is invalid or transport not HTTP Local - Clear field value and tooltip when showing validation errors - Ensure field is re-enabled when URL becomes valid
Reviewer's GuideRefines the HTTP local command UI handling for invalid/non-local URLs, adds logging when WebSocket URLs are implicitly rewritten from bind-only hosts to localhost, and introduces helper scripts for updating a forked repository from upstream. Sequence diagram for WebSocket URL build with bind-only host rewritesequenceDiagram
participant Client
participant WebSocketTransportClient
participant McpLog
Client->>WebSocketTransportClient: BuildWebSocketUri(baseUrl)
WebSocketTransportClient->>WebSocketTransportClient: Parse baseUrl as httpUri
alt Invalid baseUrl
WebSocketTransportClient->>Client: Throw InvalidOperationException
else Valid baseUrl
WebSocketTransportClient->>WebSocketTransportClient: host = httpUri.Host
alt Host is bind-only (0.0.0.0 or ::)
WebSocketTransportClient->>McpLog: Warn [WebSocket] Base URL host is bind-only, using localhost
WebSocketTransportClient->>WebSocketTransportClient: host = localhost
else Host is normal
WebSocketTransportClient->>WebSocketTransportClient: Keep original host
end
WebSocketTransportClient->>WebSocketTransportClient: Build ws/wss Uri with host
WebSocketTransportClient->>Client: Return WebSocket Uri
end
Flow diagram for update_fork helper scriptsflowchart TD
A[Run update_fork script] --> B[git checkout main]
B --> C[git fetch -ap upstream]
C --> D[git fetch -ap origin]
D --> E[git rebase upstream/main]
E --> F[git push]
F --> G[Fork main branch updated from upstream]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- The new
update_forkscripts assume anupstreamremote and amainbranch exist; consider adding checks (or parameters) so they fail with a clear message when those preconditions are not met instead of exiting with a generic git error. - The logic for detecting/handling bind-only hosts (
0.0.0.0,::) inBuildWebSocketUriand the HTTP-local URL validation in the editor code are conceptually similar; consider centralizing the allowed/denied host checks to avoid these rules drifting apart over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `update_fork` scripts assume an `upstream` remote and a `main` branch exist; consider adding checks (or parameters) so they fail with a clear message when those preconditions are not met instead of exiting with a generic git error.
- The logic for detecting/handling bind-only hosts (`0.0.0.0`, `::`) in `BuildWebSocketUri` and the HTTP-local URL validation in the editor code are conceptually similar; consider centralizing the allowed/denied host checks to avoid these rules drifting apart over time.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs:688-690` </location>
<code_context>
+ // Replace bind-only addresses with localhost for client connections
+ // 0.0.0.0 and :: are only valid for server binding, not client connections
+ string host = httpUri.Host;
+ if (host == "0.0.0.0" || host == "::")
+ {
+ McpLog.Warn($"[WebSocket] Base URL host '{host}' is bind-only; using 'localhost' for client connection.");
+ host = "localhost";
+ }
</code_context>
<issue_to_address>
**suggestion:** Warning log on bind-only host might be too noisy for common configurations
Since `0.0.0.0`/`::` is a very common bind address, this warning will fire on every WebSocket connection and clutter logs, even though the situation is handled by remapping to `localhost`. Consider reducing it to info/debug level, or ensuring it only logs once (e.g., per process or per URL) instead of per connection.
</issue_to_address>
### Comment 2
<location> `tools/update_fork.sh:5-6` </location>
<code_context>
+git checkout main
+if errorlevel 1 exit /b 1
+
+git fetch -ap upstream
+if errorlevel 1 exit /b 1
+
+git fetch -ap
+if errorlevel 1 exit /b 1
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid redundant fetch/prune operations in update script
Running `git fetch -ap upstream` followed by `git fetch -ap` performs two full fetch/prune cycles. One `git fetch -ap` is enough to update and prune all remotes (including `upstream`), or you can just fetch `upstream` if that’s the only remote needed here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (host == "0.0.0.0" || host == "::") | ||
| { | ||
| McpLog.Warn($"[WebSocket] Base URL host '{host}' is bind-only; using 'localhost' for client connection."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Warning log on bind-only host might be too noisy for common configurations
Since 0.0.0.0/:: is a very common bind address, this warning will fire on every WebSocket connection and clutter logs, even though the situation is handled by remapping to localhost. Consider reducing it to info/debug level, or ensuring it only logs once (e.g., per process or per URL) instead of per connection.
| git fetch -ap upstream | ||
| git fetch -ap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Avoid redundant fetch/prune operations in update script
Running git fetch -ap upstream followed by git fetch -ap performs two full fetch/prune cycles. One git fetch -ap is enough to update and prune all remotes (including upstream), or you can just fetch upstream if that’s the only remote needed here.
* Log a message with implicit URI changes Small update for CoplayDev#542 * Log a message with implicit URI changes Small update for CoplayDev#542 * Add helper scripts to update forks * fix: improve HTTP Local URL validation UX and styling specificity - Rename CSS class from generic "error" to "http-local-url-error" for better specificity - Rename "invalid-url" class to "http-local-invalid-url" for clarity - Disable httpServerCommandField when URL is invalid or transport not HTTP Local - Clear field value and tooltip when showing validation errors - Ensure field is re-enabled when URL becomes valid
* feat: Add CLI for Unity MCP server - Add click-based CLI with 15+ command groups - Commands: gameobject, component, scene, asset, script, editor, prefab, material, lighting, ui, audio, animation, code - HTTP transport to communicate with Unity via MCP server - Output formats: text, json, table - Configuration via environment variables or CLI options - Comprehensive usage guide and unit tests * Update based on AI feedback * Fixes main.py error * Update for further error fix * Update based on AI * Update script.py * Update with better coverage and Tool Readme * Log a message with implicit URI changes Small update for #542 * Minor fixes (#602) * Log a message with implicit URI changes Small update for #542 * Log a message with implicit URI changes Small update for #542 * Add helper scripts to update forks * fix: improve HTTP Local URL validation UX and styling specificity - Rename CSS class from generic "error" to "http-local-url-error" for better specificity - Rename "invalid-url" class to "http-local-invalid-url" for clarity - Disable httpServerCommandField when URL is invalid or transport not HTTP Local - Clear field value and tooltip when showing validation errors - Ensure field is re-enabled when URL becomes valid * Docker mcp gateway (#603) * Log a message with implicit URI changes Small update for #542 * Update docker container to default to stdio Replaces #541 * fix: Rider config path and add MCP registry manifest (#604) - Fix RiderConfigurator to use correct GitHub Copilot config path: - Windows: %LOCALAPPDATA%\github-copilot\intellij\mcp.json - macOS: ~/Library/Application Support/github-copilot/intellij/mcp.json - Linux: ~/.config/github-copilot/intellij/mcp.json - Add mcp.json for GitHub MCP Registry support: - Enables users to install via coplaydev/unity-mcp - Uses uvx with mcpforunityserver from PyPI * Use click.echo instead of print statements * Standardize whitespace * Minor tweak in docs * Use `wait` params * Unrelated but project scoped tools should be off by default * Update lock file * Whitespace cleanup * Update custom_tool_service.py to skip global registration for any tool name that already exists as a built‑in. * Avoid silently falling back to the first Unity session when a specific unity_instance was requested but not found. If a client passes a unity_instance that doesn’t match any session, this code will still route the command to the first available session, which can send commands to the wrong project in multi‑instance environments. Instead, when a unity_instance is provided but no matching session_id is found, return an error (e.g. 400/404 with "Unity instance '' not found") and only default to the first session when no unity_instance was specified. Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * Update docs/CLI_USAGE.md Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * Updated the CLI command registration to only swallow missing optional modules and to surface real import-time failures, so broken command modules don’t get silently ignored. * Sorted __all__ alphabetically to satisfy RUF022 in __init__.py. * Validate --params is a JSON object before merging. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Co-authored-by: dsarno <david@lighthaus.us> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
It was quicker to make some minor adjustments after the PRs were merged in than to go back and forth with the devs
Type of Change
Changes Made
Testing/Screenshots/Recordings
N/A
Related Issues
#542
#587
Additional Notes
Summary by Sourcery
Improve handling of local HTTP and WebSocket connection edge cases and add helper scripts for keeping forks up to date.
Enhancements:
Chores: