Add timeout settings for the WebSocket and the test execution. #22
Conversation
… for requests to the Unity Editor Bridge.
📝 WalkthroughWalkthroughThis update modifies the test execution timeout handling by removing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UnityEditor
participant McpUnitySettings
participant TestRunnerService
User->>UnityEditor: Open Server Window
UnityEditor->>McpUnitySettings: Get RequestTimeoutSeconds
User->>UnityEditor: Set new Request Timeout (seconds)
UnityEditor->>McpUnitySettings: Update RequestTimeoutSeconds
User->>UnityEditor: Run tests
UnityEditor->>TestRunnerService: ExecuteTests(testMode, testFilter, completionSource)
TestRunnerService->>McpUnitySettings: Get RequestTimeoutSeconds
TestRunnerService->>TestRunnerService: Use timeout for test execution
sequenceDiagram
participant Env
participant McpUnityServer
Env->>McpUnityServer: Provide UNITY_REQUEST_TIMEOUT (optional)
McpUnityServer->>McpUnityServer: Set REQUEST_TIMEOUT (from env or default)
McpUnityServer->>Log: Log effective request timeout
Client->>McpUnityServer: Send request
McpUnityServer->>McpUnityServer: Use REQUEST_TIMEOUT for request handling
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Editor/UnityBridge/McpUnityEditorWindow.cs (1)
116-116: Consider using the logger service for consistent error handling.Instead of using
Debug.LogErrordirectly, consider using the logger service to maintain consistent error logging throughout the application.- Debug.LogError("Test timeout must be at least 60 seconds."); + Logger.LogError("Test timeout must be at least 60 seconds.");Server/build/unity/mcpUnity.js (1)
23-25: Add validation for the environment variable value.The current implementation lacks validation for non-numeric values in the
UNITY_REQUEST_TIMEOUTenvironment variable. While JavaScript's behavior withparseInt()returningNaNfor non-numeric values would likely result in the default being used (sinceNaNis falsy), explicit validation would make the code more robust.// Initialize timeout from environment variable (in seconds; it is the same as Cline) or use default (10 seconds) const envTimeout = process.env.UNITY_REQUEST_TIMEOUT; - this.REQUEST_TIMEOUT = envTimeout ? parseInt(envTimeout, 10) * 1000 : 10000; + const parsedTimeout = envTimeout ? parseInt(envTimeout, 10) : NaN; + this.REQUEST_TIMEOUT = !isNaN(parsedTimeout) ? parsedTimeout * 1000 : 10000; this.logger.info(`Using request timeout: ${this.REQUEST_TIMEOUT / 1000} seconds`);README-ja.md (4)
214-214: セクション見出しの統一性
他のセクションと同様に、アンカー（例：<a name="timeout-settings"></a>）を設定し、目次やリンクから直接参照できるようにすると親切です。また、日本語文中の区切り文字は全角コロン（：）に統一することを検討してください。
216-223: デフォルト値の強調
「デフォルトでは、MCPサーバーとWebSocket間のタイムアウトは 10 秒です。」という記述がありますが、値を太字やコード形式（10 秒）で強調すると、一目で分かりやすくなります。
238-240: 日本語表現の改善提案
「AIコーディングIDE」という表現が繰り返されてやや冗長です。例えば「使用する AI支援IDE によって異なります」などのほうが自然かと思います。
241-244: UIラベルとの整合性確認
「Test Timeout (seconds)」という英語表記をそのまま掲載していますが、Unityエディター側の実際のラベル（英語or日本語）と一致しているかご確認ください。日本語UIに合わせる場合は「テストタイムアウト（秒）」などに翻訳することを検討してください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Editor/Services/ITestRunnerService.cs(1 hunks)Editor/Services/TestRunnerService.cs(2 hunks)Editor/UnityBridge/McpUnityEditorWindow.cs(1 hunks)Editor/UnityBridge/McpUnitySettings.cs(2 hunks)README-ja.md(1 hunks)README.md(1 hunks)README_zh-CN.md(1 hunks)Server/build/unity/mcpUnity.js(2 hunks)Server/src/unity/mcpUnity.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Editor/UnityBridge/McpUnityEditorWindow.cs (2)
Editor/Utils/Logger.cs (1)
LogError(38-41)Editor/UnityBridge/McpUnitySettings.cs (1)
SaveSettings(90-107)
🔇 Additional comments (13)
Editor/UnityBridge/McpUnitySettings.cs (1)
32-33: Good implementation of the test timeout setting.The new
TestTimeoutSecondsfield with a default value of 60 seconds is well-documented with an appropriate tooltip. This aligns with the PR objective of creating a configurable timeout setting for test execution.Editor/UnityBridge/McpUnityEditorWindow.cs (1)
110-127: Well-implemented UI for the test timeout setting.The implementation correctly:
- Displays the current timeout value
- Validates input to ensure a minimum of 60 seconds
- Provides appropriate error feedback
- Saves changes to the persistent settings
This provides a good user experience for configuring the test execution timeout.
Editor/Services/ITestRunnerService.cs (1)
30-33: Interface correctly updated to remove the timeout parameter.The
ExecuteTestsmethod signature has been updated to remove thetimeoutMinutesparameter, which is now replaced by the globalTestTimeoutSecondssetting. This change is consistent with the PR objective to make test execution timeout configurable.Server/build/unity/mcpUnity.js (1)
11-25: Good implementation of configurable WebSocket timeout.The WebSocket timeout is now properly configurable via the
UNITY_REQUEST_TIMEOUTenvironment variable, with a sensible default of 10 seconds. The logging of the configured timeout value is helpful for debugging.README_zh-CN.md (1)
216-247: Documentation properly explains the new timeout configuration options.The added section clearly explains both timeout settings:
- WebSocket timeout: Configurable via
UNITY_REQUEST_TIMEOUTenvironment variable with a helpful JSON example- Test execution timeout: Configurable through the Unity Editor UI
The tip about IDE-specific timeouts is also valuable contextual information for users.
README.md (1)
239-270: Clear documentation of new timeout configuration options.The added section effectively communicates:
- WebSocket timeout configuration via environment variable with a useful JSON example
- Test execution timeout configuration through the Unity Editor UI
- Default values for both settings
The tip about IDE timeouts provides important context to prevent confusion about different timeout behaviors.
Editor/Services/TestRunnerService.cs (4)
56-56: Method signature updated to remove the timeout parameter.The
ExecuteTestsmethod signature has been simplified by removing thetimeoutMinutesparameter, which is now retrieved from settings instead.
73-75: Properly retrieving timeout from central configuration.Using
McpUnitySettings.Instance.TestTimeoutSecondscentralizes the timeout configuration, ensuring consistency across the application.
78-78: Using seconds instead of minutes for timeout units.Changed from
FromMinutestoFromSecondsto match the settings value's unit.
84-84: Updated error message to reflect timeout unit change.The error message now correctly refers to seconds instead of minutes, maintaining consistency with the configuration unit.
Server/src/unity/mcpUnity.ts (2)
36-36: Changed timeout from constant to instance property.Good change to make the timeout configurable rather than hardcoded.
51-54: WebSocket timeout now configurable via environment variable.The implementation properly:
- Reads timeout from the
UNITY_REQUEST_TIMEOUTenvironment variable- Converts from seconds to milliseconds (matching the client-side unit)
- Provides a sensible default of 10 seconds
- Includes a clear comment about the units and default
This allows users to configure the timeout without modifying code.
README-ja.md (1)
221-236: 設定例のポート番号の一貫性確認
JSONスニペット内でUNITY_PORT: "8090"としていますが、トラブルシューティングセクションではデフォルトポートが 8080 と案内されています。読者の混乱を避けるため、例示ポートをデフォルトの8080に揃えるか、意図的に8090を使う理由を追記してください。
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Warning README_zh-CN.md is translated by Google Translate. |
|
Interesting PR @nowsprinting My suggestion is to change the name of the field to "Request timeout" instead of "Test Timeout" |
|
@CoderGamester fixed! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
README.md (3)
239-239: Clarify section title to explicitly reference "Request Timeout"
The heading “Optional: Set Timeout” is ambiguous—consider renaming it to “Optional: Set Request Timeout” to match the terminology used elsewhere (e.g., “Request Timeout (seconds)”).- ## Optional: Set Timeout + ## Optional: Set Request Timeout
250-250: Fix verb usage: use “set up” instead of “setup”
“Setup” is a noun; the correct verb phrase is “set up.”- Unity will setup the system environment variable UNITY_REQUEST_TIMEOUT to the new timeout value + Unity will set up the system environment variable UNITY_REQUEST_TIMEOUT to the new timeout value🧰 Tools
🪛 LanguageTool
[grammar] ~250-~250: The word “setup” is a noun. The verb is spelled with a space.
Context: ...r desired timeout seconds 4. Unity will setup the system environment variable UNITY_R...(NOUN_VERB_CONFUSION)
269-277: Add Unix shell example for setting the environment variable
Under “WebSocket Timeout,” non-Windows users should see a standard shell (bash/zsh) example usingexportrather than only PowerShell/set.- 1. Set the UNITY_REQUEST_TIMEOUT environment variable in the terminal - - Powershell - ```powershell - $env:UNITY_REQUEST_TIMEOUT = "300" - ``` - - Command Prompt/Terminal - ```cmd - set UNITY_REQUEST_TIMEOUT=300 - ``` + 1. Set the UNITY_REQUEST_TIMEOUT environment variable in your shell: + ```bash + export UNITY_REQUEST_TIMEOUT=300 + ``` + 2. (Windows users may still use PowerShell or CMD as documented above.)README_zh-CN.md (3)
216-219: Refine section heading for clarity
建议将“## 可选：设置超时”调整为“## 可选：设置请求超时”，以与后续“Request Timeout (seconds)”字段保持一致，并更清晰地表述该超时针对的是请求层面。
221-230: Clarify environment variable persistence & localize option labels
- 当前步骤写到“Unity 会将系统环境变量 UNITY_REQUEST_TIMEOUT 设置为新的超时值”，但此操作只影响 Unity 进程及其子进程，且为会话级别。建议补充如何在 Windows 系统环境变量面板中永久设置，或直接通过编辑器界面调整并避免环境变量困扰。
<summary>标签中“Option 1: Windows OS”为英文混排，建议本地化为“选项 1：Windows 系统”，提升文档连贯性。🧰 Tools
🪛 LanguageTool
[uncategorized] ~227-~227: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：新"地"超时
Context: ...nity 会将系统环境变量 UNITY_REQUEST_TIMEOUT 设置为新的超时值 5. 重启 Node.js 服务器 6. 再次点击“启动服务器”，将 U...(wb4)
233-257: 补充 macOS/Linux 的环境变量设置示例
非 Windows 系统部分仅覆盖了 PowerShell 和 CMD，未说明常见的 Bash/Zsh 方式。建议增加：# 临时（当前终端会话） export UNITY_REQUEST_TIMEOUT=300 # 永久（写入 ~/.bash_profile 或 ~/.zshrc） echo 'export UNITY_REQUEST_TIMEOUT=300' >> ~/.bash_profile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Editor/Services/TestRunnerService.cs(2 hunks)Editor/UnityBridge/McpUnityEditorWindow.cs(1 hunks)Editor/UnityBridge/McpUnitySettings.cs(3 hunks)README-ja.md(1 hunks)README.md(1 hunks)README_zh-CN.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Editor/UnityBridge/McpUnityEditorWindow.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Editor/Services/TestRunnerService.cs
- README-ja.md
- Editor/UnityBridge/McpUnitySettings.cs
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~250-~250: The word “setup” is a noun. The verb is spelled with a space.
Context: ...r desired timeout seconds 4. Unity will setup the system environment variable UNITY_R...
(NOUN_VERB_CONFUSION)
README_zh-CN.md
[uncategorized] ~227-~227: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：新"地"超时
Context: ...nity 会将系统环境变量 UNITY_REQUEST_TIMEOUT 设置为新的超时值 5. 重启 Node.js 服务器 6. 再次点击“启动服务器”，将 U...
(wb4)
| ## 可选：设置超时 | ||
|
|
||
| 默认情况下，MCP 服务器与 WebSocket 之间的超时时间为 10 秒。 | ||
| 您可以根据所使用的操作系统进行更改： | ||
|
|
||
| <details> | ||
| <summary><span style="font-size: 1.1em; font-weight: bold;">Option 1: Windows OS</span></summary> | ||
|
|
||
| 1. 打开 Unity 编辑器 | ||
| 2. 导航至 Tools > MCP Unity > Server Window | ||
| 3. 将 "Request Timeout (seconds)" 值更改为所需的超时秒数 | ||
| 4. Unity 会将系统环境变量 UNITY_REQUEST_TIMEOUT 设置为新的超时值 | ||
| 5. 重启 Node.js 服务器 | ||
| 6. 再次点击“启动服务器”，将 Unity 编辑器 Web 套接字重新连接到 Node.js MCP 服务器 | ||
|
|
||
| </details> | ||
|
|
||
| <details> | ||
| <summary><span style="font-size: 1.1em; font-weight: bold;">Option 2: 非Windows操作系统</span></summary> | ||
|
|
||
| 对于非Windows操作系统，需要配置两个地方： | ||
|
|
||
| ### 编辑器进程超时 | ||
|
|
||
| 1. 打开 Unity 编辑器 | ||
| 2. 导航至 Tools > MCP Unity > Server Window | ||
| 3. 将 "Request Timeout (seconds)" 值更改为所需的超时秒数 | ||
|
|
||
| ### WebSocket 超时 | ||
|
|
||
| 1. 在终端中设置 UNITY_REQUEST_TIMEOUT 环境变量 | ||
| - Powershell | ||
| ```powershell | ||
| $env:UNITY_REQUEST_TIMEOUT = "300" | ||
| ``` | ||
| - Command Prompt/Terminal | ||
| ```cmd | ||
| set UNITY_REQUEST_TIMEOUT=300 | ||
| ``` | ||
| 2. 重启 Node.js 服务器 | ||
| 3. 再次点击“启动服务器”，将 Unity 编辑器 Web 套接字重新连接到 Node.js MCP 服务器 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
统一通过编辑器界面配置超时
为简化用户体验，可考虑移除非 Windows 环境下对 UNITY_REQUEST_TIMEOUT 的手动设置，仅在 “Request Timeout (seconds)” 字段中统一管理所有请求（包括测试执行与 WebSocket）。这样可避免跨平台环境变量差异，且无需重启或手工维护。
🧰 Tools
🪛 LanguageTool
[uncategorized] ~227-~227: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：新"地"超时
Context: ...nity 会将系统环境变量 UNITY_REQUEST_TIMEOUT 设置为新的超时值 5. 重启 Node.js 服务器 6. 再次点击“启动服务器”，将 U...
(wb4)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| // see: https://learn.microsoft.com/ja-jp/dotnet/api/system.environmentvariabletarget?view=net-8.0#fields | ||
| Environment.SetEnvironmentVariable(EnvUnityPort, Port.ToString(), EnvironmentVariableTarget.User); | ||
| Environment.SetEnvironmentVariable(EnvUnityRequestTimeout, RequestTimeoutSeconds.ToString(), EnvironmentVariableTarget.User); | ||
| #endif |
There was a problem hiding this comment.
doesn't this code run on Mac machines?
Asking as I don't have one at the moment to test
There was a problem hiding this comment.
On a macOS, it behaves the same as Process.
So, I use SerializeField to persist data when not on Windows.
There was a problem hiding this comment.
I can confirm it works properly on windows.
This needs to have a new solution for Mac, but that should be done in a separate PR as it was a problem present before
There was a problem hiding this comment.
Good work
Approved and @nowsprinting can be merged
If not merged by tomorrow I will do merge by the end of the day
| // see: https://learn.microsoft.com/ja-jp/dotnet/api/system.environmentvariabletarget?view=net-8.0#fields | ||
| Environment.SetEnvironmentVariable(EnvUnityPort, Port.ToString(), EnvironmentVariableTarget.User); | ||
| Environment.SetEnvironmentVariable(EnvUnityRequestTimeout, RequestTimeoutSeconds.ToString(), EnvironmentVariableTarget.User); | ||
| #endif |
There was a problem hiding this comment.
I can confirm it works properly on windows.
This needs to have a new solution for Mac, but that should be done in a separate PR as it was a problem present before
|
@CoderGamester Thank you for your review! The owner must merge. |
Changes
Add WebSocket timeout setting
Can specify it as an environment variable
UNITY_REQUEST_TIMEOUTin the MCP configuration file as follows:{ "mcpServers": { "mcp-unity": { "command": "node", "args": [ "ABSOLUTE/PATH/TO/mcp-unity/Server/build/index.js" ], "env": { "UNITY_PORT": "8090", "UNITY_REQUEST_TIMEOUT": "300" } } } }Add test execution timeout setting
Can specify it in Request Timeout (seconds) in the Tools > MCP Unity > Server Window.
Summary by CodeRabbit
New Features
UNITY_REQUEST_TIMEOUTenvironment variable.Documentation
Bug Fixes