Add --offline to uvx launches for faster startup (#760)#768
Merged
dsarno merged 4 commits intoCoplayDev:betafrom Feb 17, 2026
Merged
Add --offline to uvx launches for faster startup (#760)#768dsarno merged 4 commits intoCoplayDev:betafrom
dsarno merged 4 commits intoCoplayDev:betafrom
Conversation
…arm (CoplayDev#760) When the uvx cache already has the package, pass --offline to skip the network dependency check that can hang for 30+ seconds on poor connections. A lightweight probe (uvx --offline ... --help) with a 3-second timeout determines cache warmth before building the command. Also fixes stale LogAssert expectations in SetComponentProperties test to match current error message format from ComponentOps refactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements conditional use of Sequence diagram for uvx command construction with offline/refresh flagssequenceDiagram
actor Developer
participant UnityEditor as UnityEditor_UI
participant McpClientConfigSection
participant AssetPathUtility
participant McpClientConfiguratorBase
Developer->>UnityEditor: Open MCP client configuration
UnityEditor->>McpClientConfigSection: ConfigureClaudeCliAsync(client)
McpClientConfigSection->>AssetPathUtility: GetUvxCommandParts()
AssetPathUtility-->>McpClientConfigSection: uvxPath, fromArgs, packageName
McpClientConfigSection->>AssetPathUtility: ShouldForceUvxRefresh()
AssetPathUtility-->>McpClientConfigSection: shouldForceRefresh
McpClientConfigSection->>AssetPathUtility: ShouldUseUvxOffline()
AssetPathUtility-->>McpClientConfigSection: shouldUseOffline
McpClientConfigSection->>McpClientConfiguratorBase: ConfigureWithCapturedValues(..., uvxPath, fromArgs, packageName, shouldForceRefresh, shouldUseOffline, apiKey, serverTransport)
activate McpClientConfiguratorBase
McpClientConfiguratorBase->>McpClientConfiguratorBase: RegisterWithCapturedValues(..., shouldForceRefresh, shouldUseOffline,...)
alt shouldForceRefresh
McpClientConfiguratorBase->>McpClientConfiguratorBase: devFlags = "--no-cache --refresh "
else shouldUseOffline
McpClientConfiguratorBase->>McpClientConfiguratorBase: devFlags = "--offline "
else
McpClientConfiguratorBase->>McpClientConfiguratorBase: devFlags = ""
end
McpClientConfiguratorBase-->>Developer: uvx command with appropriate flags
Class diagram for uvx offline flag propagationclassDiagram
class AssetPathUtility {
+bool ShouldForceUvxRefresh()
+bool ShouldUseUvxOffline()
+string GetUvxPath()
+string GetBetaServerFromArgs(bool quoteFromPath)
+List~string~ GetBetaServerFromArgsList()
}
class CodexConfigHelper {
+string BuildCodexServerBlock(string uvPath)
+TomlTable CreateUnityMcpTable(string uvPath)
-void AddUvxModeFlags(TomlArray args)
}
class ServerCommandBuilder {
+bool TryBuildCommand(out string fileName, out string arguments, out string workingDirectory)
-string BuildUvxDevFlags()
}
class ConfigJsonBuilder {
-IList~string~ BuildUvxArgs(string fromUrl, string packageName)
}
class McpClientConfigSection {
-void ConfigureClaudeCliAsync(IMcpClientConfigurator client)
}
class IMcpClientConfigurator {
<<interface>>
+void ConfigureWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string fromArgs, string packageName, bool shouldForceRefresh, bool shouldUseOffline, string apiKey, Models_ConfiguredTransport serverTransport)
}
class McpClientConfiguratorBase {
+void ConfigureWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string fromArgs, string packageName, bool shouldForceRefresh, bool shouldUseOffline, string apiKey, Models_ConfiguredTransport serverTransport)
-void RegisterWithCapturedValues(string projectDir, string claudePath, string pathPrepend, bool useHttpTransport, string httpUrl, string uvxPath, string fromArgs, string packageName, bool shouldForceRefresh, bool shouldUseOffline, string apiKey, Models_ConfiguredTransport serverTransport)
-void Register()
+string GetManualSnippet()
}
AssetPathUtility <.. CodexConfigHelper : uses
AssetPathUtility <.. ServerCommandBuilder : uses
AssetPathUtility <.. ConfigJsonBuilder : uses
AssetPathUtility <.. McpClientConfigSection : uses
McpClientConfigSection --> IMcpClientConfigurator : configures
McpClientConfiguratorBase ..|> IMcpClientConfigurator : implements
McpClientConfigSection ..> McpClientConfiguratorBase : typical implementation
CodexConfigHelper ..> AssetPathUtility : mode flags
ServerCommandBuilder ..> AssetPathUtility : mode flags
ConfigJsonBuilder ..> AssetPathUtility : mode flags
McpClientConfiguratorBase ..> AssetPathUtility : mode flags
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughCentralizes UVX dev-flag derivation and offline probing via new AssetPathUtility APIs, replaces scattered shouldForceRefresh heuristics with a Changes
Sequence DiagramsequenceDiagram
participant User
participant McpClientConfigSection
participant AssetPathUtility
participant ConfigBuilders as ConfigJsonBuilder/ServerCommandBuilder
participant UVX as UVX Service
User->>McpClientConfigSection: Trigger Claude CLI configuration
McpClientConfigSection->>AssetPathUtility: GetUvxDevFlags()
AssetPathUtility->>UVX: (optional) Run offline probe (<=3s)
UVX-->>AssetPathUtility: Probe result
AssetPathUtility-->>McpClientConfigSection: Return uvxDevFlags
McpClientConfigSection->>ConfigBuilders: Pass uvxDevFlags into Register/Build flows
ConfigBuilders->>AssetPathUtility: GetUvxDevFlagsList()
ConfigBuilders-->>UVX: Start/register using constructed args
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
devFlagsselection logic (--no-cache --refreshvs--offlinevs empty) is duplicated acrossServerCommandBuilder,McpClientConfiguratorBase(multiple spots), andConfigJsonBuilder—consider extracting a shared helper inAssetPathUtility(or similar) to keep the behavior consistent and easier to adjust. ShouldUseUvxOffline()may be called multiple times in a session (e.g., config JSON, server command, registration, manual snippet), each time spawning auvxprocess; consider memoizing the probe result per editor session or for a short interval to avoid repeated external calls and potential 3-second timeouts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `devFlags` selection logic (`--no-cache --refresh` vs `--offline` vs empty) is duplicated across `ServerCommandBuilder`, `McpClientConfiguratorBase` (multiple spots), and `ConfigJsonBuilder`—consider extracting a shared helper in `AssetPathUtility` (or similar) to keep the behavior consistent and easier to adjust.
- `ShouldUseUvxOffline()` may be called multiple times in a session (e.g., config JSON, server command, registration, manual snippet), each time spawning a `uvx` process; consider memoizing the probe result per editor session or for a short interval to avoid repeated external calls and potential 3-second timeouts.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs:795-801` </location>
<code_context>
{
// Note: --reinstall is not supported by uvx, use --no-cache --refresh instead
- string devFlags = shouldForceRefresh ? "--no-cache --refresh " : string.Empty;
+ string devFlags;
+ if (shouldForceRefresh)
+ devFlags = "--no-cache --refresh ";
+ else if (shouldUseOffline)
+ devFlags = "--offline ";
+ else
+ devFlags = string.Empty;
// Use --scope local to register in the project-local config, avoiding conflicts with user-level config (#664)
args = $"mcp add --scope local --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}{fromArgs} {packageName}";
</code_context>
<issue_to_address>
**suggestion:** Centralize `uvx` flag selection logic instead of duplicating `devFlags` branches.
This `devFlags` selection (between `--no-cache --refresh`, `--offline`, or nothing) now appears here, in `McpClientConfiguratorBase.Register()`, `ServerCommandBuilder.TryBuildCommand`, and in the `ConfigJsonBuilder.BuildUvxArgs` / `CodexConfigHelper.AddUvxModeFlags` pair. Consider extracting a shared helper (e.g., `AssetPathUtility.GetUvxDevFlags()` returning a string or string list) so flag precedence and behavior are defined in one place instead of being manually kept in sync across multiple branches.
Suggested implementation:
```csharp
// Note: --reinstall is not supported by uvx, use --no-cache --refresh instead.
// Use shared helper so uvx flag precedence is defined in one place.
var devFlags = AssetPathUtility.GetUvxDevFlags(
forceRefresh: shouldForceRefresh,
useOffline: shouldUseOffline);
// Use --scope local to register in the project-local config, avoiding conflicts with user-level config (#664)
args = $"mcp add --scope local --transport stdio UnityMCP -- \"{uvxPath}\" {devFlags}{fromArgs} {packageName}";
```
```csharp
var (uvxPath, _, packageName) = AssetPathUtility.GetUvxCommandParts();
// Use central helper that checks both DevModeForceServerRefresh AND local path detection.
var devFlags = AssetPathUtility.GetUvxDevFlags(
forceRefresh: AssetPathUtility.ShouldForceUvxRefresh(),
useOffline: AssetPathUtility.ShouldUseUvxOffline());
```
To fully implement the review suggestion, you should also:
1. Add a shared helper to `AssetPathUtility` (e.g. in `MCPForUnity/Editor/Utilities/AssetPathUtility.cs` or wherever it lives):
```csharp
public static string GetUvxDevFlags(bool forceRefresh, bool useOffline)
{
// Note: --reinstall is not supported by uvx, use --no-cache --refresh instead
if (forceRefresh)
return "--no-cache --refresh ";
if (useOffline)
return "--offline ";
return string.Empty;
}
```
If some call sites should infer `forceRefresh` / `useOffline` from global settings, you can optionally add a parameterless overload:
```csharp
public static string GetUvxDevFlags()
=> GetUvxDevFlags(ShouldForceUvxRefresh(), ShouldUseUvxOffline());
```
2. Update other locations that currently duplicate the uvx flag logic (`ServerCommandBuilder.TryBuildCommand`, `ConfigJsonBuilder.BuildUvxArgs`, `CodexConfigHelper.AddUvxModeFlags`, and any others) to call `AssetPathUtility.GetUvxDevFlags(...)` instead of hard-coding the `--no-cache --refresh` / `--offline` branching.
3. Ensure all usages preserve the desired precedence (force-refresh over offline) by relying on the centralized helper rather than local if/else chains.
These changes will make uvx flag behavior consistent and easier to modify in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/AssetPathUtilityOfflineTests.cs (1)
24-36: Test coverage is minimal — consider adding a null-uvx-path scenario.The existing tests verify the force-refresh guard and the no-throw guarantee, which are good baseline checks. However, neither test verifies the
falsereturn whenGetUvxPath()returns null/empty (a common CI/test-environment condition). Since thePathResolverServiceis registered viaMCPServiceLocator, you could register a mock that returnsnullto cover that branch without needing a realuvxbinary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/AssetPathUtilityOfflineTests.cs` around lines 24 - 36, Add a test in AssetPathUtilityOfflineTests that registers a mock PathResolverService with MCPServiceLocator which returns null (or empty) from GetUvxPath, ensure EditorPrefKeys.DevModeForceServerRefresh is false, then assert AssetPathUtility.ShouldUseUvxOffline() returns false to cover the null-uvx-path branch; use the existing test harness patterns for registering services in MCPServiceLocator so the test cleans up after itself.MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
459-462: Verify thatGetBetaServerFromArgs(EditorPrefs access) is safe here.
GetBetaServerFromArgs()at Line 459 readsEditorPrefsand must be called from the main Unity thread. This is currently true for all direct callers. However, inMcpClientConfigSection.ConfigureClaudeCliAsync, the result ofShouldUseUvxOffline()is captured beforeTask.Run(Line 308), which is correct. Just ensure no future caller invokes this from a background thread.The method's XML doc should note the main-thread requirement, consistent with how
GetBetaServerFromArgs()documents this constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 459 - 462, GetBetaServerFromArgs reads EditorPrefs and must only be called from Unity's main thread; update the XML doc on GetBetaServerFromArgs to explicitly state the main-thread requirement, and add a brief XML note on ShouldUseUvxOffline (and/or the AssetPathUtility method that calls it) indicating callers must invoke it on the main thread (or capture its result before spawning background tasks). Also verify McpClientConfigSection.ConfigureClaudeCliAsync already captures ShouldUseUvxOffline before Task.Run and add a comment referencing ConfigureClaudeCliAsync to remind future maintainers to preserve that pattern.
448-470: Multiple callers may spawn redundant probe processes.
ShouldUseUvxOffline()spawns a subprocess with a 3-second timeout. It's called independently fromConfigJsonBuilder.BuildUvxArgs,ServerCommandBuilder.TryBuildCommand,CodexConfigHelper.AddUvxModeFlags(×2),McpClientConfiguratorBase.Register(), andGetManualSnippet(). In flows like "Configure All Clients", several of these paths may execute in quick succession, each spawning a fresh probe process.Consider caching the probe result for a short duration (e.g., 30–60 seconds) so that repeated calls within the same configuration flow reuse the first result instead of spawning a new process each time.
💡 Example: lightweight time-based cache
+ private static bool _cachedOfflineResult; + private static DateTime _cachedOfflineTimestamp = DateTime.MinValue; + private static readonly TimeSpan OfflineCacheDuration = TimeSpan.FromSeconds(30); + public static bool ShouldUseUvxOffline() { if (ShouldForceUvxRefresh()) return false; + if ((DateTime.UtcNow - _cachedOfflineTimestamp) < OfflineCacheDuration) + return _cachedOfflineResult; + try { string uvxPath = MCPServiceLocator.Paths.GetUvxPath(); if (string.IsNullOrEmpty(uvxPath)) return false; string fromArgs = GetBetaServerFromArgs(quoteFromPath: false); string probeArgs = string.IsNullOrEmpty(fromArgs) ? "--offline mcp-for-unity --help" : $"--offline {fromArgs} mcp-for-unity --help"; - return ExecPath.TryRun(uvxPath, probeArgs, null, out _, out _, timeoutMs: 3000); + _cachedOfflineResult = ExecPath.TryRun(uvxPath, probeArgs, null, out _, out _, timeoutMs: 3000); + _cachedOfflineTimestamp = DateTime.UtcNow; + return _cachedOfflineResult; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 448 - 470, ShouldUseUvxOffline currently launches a short-lived probe process each call, causing redundant probes when multiple callers (ConfigJsonBuilder.BuildUvxArgs, ServerCommandBuilder.TryBuildCommand, CodexConfigHelper.AddUvxModeFlags, McpClientConfiguratorBase.Register, GetManualSnippet) call it; add a lightweight time-based cache inside AssetPathUtility to store the last probe result and timestamp (e.g., 30–60s) and return the cached boolean if still fresh. Implement cache invalidation when ShouldForceUvxRefresh() returns true or when MCPServiceLocator.Paths.GetUvxPath() returns a different path than the one used for the cached probe. Ensure the cache is stored in static fields and access is thread-safe (lock or Interlocked) so concurrent callers reuse the same probe result instead of spawning new subprocesses.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
996-1006:--offlinein user-facing manual snippet may confuse users who copy it to a cold-cache machine.
GetManualSnippet()bakes the current offline probe result into the displayed CLI command. If a user copies this snippet to a different machine (or after clearing the uvx cache), the--offlineflag would cause a failure. Consider appending a brief comment in the snippet noting that--offlineis auto-detected and may need removal.This is a minor UX concern — not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 996 - 1006, The manual snippet produced by GetManualSnippet currently bakes the runtime-detected "--offline" into devFlags which can break when pasted to a cold-cache machine; update GetManualSnippet so that when devFlags contains "--offline " it appends a short user-facing comment to the returned string (near the line combining uvxPath, devFlags and fromArgs) such as "(auto-detected --offline; remove if running on a fresh machine)" instead of silently embedding the flag, keeping all other behavior the same and referencing uvxPath, devFlags and fromArgs when composing the final string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 996-1006: The manual snippet produced by GetManualSnippet
currently bakes the runtime-detected "--offline" into devFlags which can break
when pasted to a cold-cache machine; update GetManualSnippet so that when
devFlags contains "--offline " it appends a short user-facing comment to the
returned string (near the line combining uvxPath, devFlags and fromArgs) such as
"(auto-detected --offline; remove if running on a fresh machine)" instead of
silently embedding the flag, keeping all other behavior the same and referencing
uvxPath, devFlags and fromArgs when composing the final string.
In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs`:
- Around line 459-462: GetBetaServerFromArgs reads EditorPrefs and must only be
called from Unity's main thread; update the XML doc on GetBetaServerFromArgs to
explicitly state the main-thread requirement, and add a brief XML note on
ShouldUseUvxOffline (and/or the AssetPathUtility method that calls it)
indicating callers must invoke it on the main thread (or capture its result
before spawning background tasks). Also verify
McpClientConfigSection.ConfigureClaudeCliAsync already captures
ShouldUseUvxOffline before Task.Run and add a comment referencing
ConfigureClaudeCliAsync to remind future maintainers to preserve that pattern.
- Around line 448-470: ShouldUseUvxOffline currently launches a short-lived
probe process each call, causing redundant probes when multiple callers
(ConfigJsonBuilder.BuildUvxArgs, ServerCommandBuilder.TryBuildCommand,
CodexConfigHelper.AddUvxModeFlags, McpClientConfiguratorBase.Register,
GetManualSnippet) call it; add a lightweight time-based cache inside
AssetPathUtility to store the last probe result and timestamp (e.g., 30–60s) and
return the cached boolean if still fresh. Implement cache invalidation when
ShouldForceUvxRefresh() returns true or when
MCPServiceLocator.Paths.GetUvxPath() returns a different path than the one used
for the cached probe. Ensure the cache is stored in static fields and access is
thread-safe (lock or Interlocked) so concurrent callers reuse the same probe
result instead of spawning new subprocesses.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/AssetPathUtilityOfflineTests.cs`:
- Around line 24-36: Add a test in AssetPathUtilityOfflineTests that registers a
mock PathResolverService with MCPServiceLocator which returns null (or empty)
from GetUvxPath, ensure EditorPrefKeys.DevModeForceServerRefresh is false, then
assert AssetPathUtility.ShouldUseUvxOffline() returns false to cover the
null-uvx-path branch; use the existing test harness patterns for registering
services in MCPServiceLocator so the test cleans up after itself.
Extract GetUvxDevFlags() and GetUvxDevFlagsList() helpers to AssetPathUtility, replacing duplicated if/else if/else blocks across 6 call sites. Add 30-second TTL cache to ShouldUseUvxOffline() to avoid redundant subprocess spawns. Simplify ConfigureWithCapturedValues signature from two bools to a single pre-captured flags string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
466-468: Consider declaring cache fields closer to the method that uses them, or grouping at class top.The static fields
_offlineCacheResult,_offlineCacheTimestamp, andOfflineCacheTtlSecondsare declared afterShouldUseUvxOffline()andRunOfflineProbe()that use them. This is valid C# but placing state after its usage can reduce readability. A minor nit — grouping them just aboveShouldUseUvxOffline()or at the top of the class would be more conventional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 466 - 468, Move the static cache fields so their declarations are colocated with the methods that use them: relocate _offlineCacheResult, _offlineCacheTimestamp, and OfflineCacheTtlSeconds to be either grouped at the top of the class with other static state or directly above the ShouldUseUvxOffline() and RunOfflineProbe() methods; update any existing comments and ensure their accessibility (static) and initializers remain unchanged so the methods (ShouldUseUvxOffline, RunOfflineProbe) continue to reference them without code changes.
442-489: Well-designed offline probe with safe fallback behavior.The cache-probe approach with TTL is solid. A few observations:
Main-thread blocking:
RunOfflineProbe()can block the Unity editor main thread for up to 3 seconds on a cold probe. With the 30s TTL this should be rare, but it could cause a noticeable hitch when the cache expires and uvx is slow to respond. Consider whether this could eventually move to a background task that pre-warms the cache.
ShouldForceUvxRefresh()is evaluated twice when called viaGetUvxDevFlags()orGetUvxDevFlagsList()— once at the call site and again insideShouldUseUvxOffline()(Line 453). Not a bug (it's a cheap EditorPrefs read), but you could avoid it by inlining the check:♻️ Optional: Avoid redundant ShouldForceUvxRefresh() calls in list variant
public static IReadOnlyList<string> GetUvxDevFlagsList() { - if (ShouldForceUvxRefresh()) return new[] { "--no-cache", "--refresh" }; - if (ShouldUseUvxOffline()) return new[] { "--offline" }; - return Array.Empty<string>(); + bool forceRefresh = ShouldForceUvxRefresh(); + if (forceRefresh) return new[] { "--no-cache", "--refresh" }; + if (!forceRefresh && ShouldUseUvxOfflineSkipRefreshCheck()) return new[] { "--offline" }; + return Array.Empty<string>(); }That said, the current implementation is correct and the overhead is negligible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 442 - 489, The issue is redundant calls to ShouldForceUvxRefresh() (once at the caller and again inside ShouldUseUvxOffline()), so update the API to avoid duplicate EditorPrefs reads by adding an overload or parameter to ShouldUseUvxOffline(e.g., ShouldUseUvxOffline(bool skipForceCheck) or ShouldUseUvxOffline(bool forceRefreshFlag) ) and change callers like GetUvxDevFlags()/GetUvxDevFlagsList() to compute ShouldForceUvxRefresh() once and pass the result into the new overload; keep existing behavior when caller doesn’t pass the flag by having the parameter default to false or calling the original path that invokes ShouldForceUvxRefresh() internally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs`:
- Around line 466-468: Move the static cache fields so their declarations are
colocated with the methods that use them: relocate _offlineCacheResult,
_offlineCacheTimestamp, and OfflineCacheTtlSeconds to be either grouped at the
top of the class with other static state or directly above the
ShouldUseUvxOffline() and RunOfflineProbe() methods; update any existing
comments and ensure their accessibility (static) and initializers remain
unchanged so the methods (ShouldUseUvxOffline, RunOfflineProbe) continue to
reference them without code changes.
- Around line 442-489: The issue is redundant calls to ShouldForceUvxRefresh()
(once at the caller and again inside ShouldUseUvxOffline()), so update the API
to avoid duplicate EditorPrefs reads by adding an overload or parameter to
ShouldUseUvxOffline(e.g., ShouldUseUvxOffline(bool skipForceCheck) or
ShouldUseUvxOffline(bool forceRefreshFlag) ) and change callers like
GetUvxDevFlags()/GetUvxDevFlagsList() to compute ShouldForceUvxRefresh() once
and pass the result into the new overload; keep existing behavior when caller
doesn’t pass the flag by having the parameter default to false or calling the
original path that invokes ShouldForceUvxRefresh() internally.
…resh calls Address CodeRabbit nitpicks: relocate static cache fields above the methods that use them for readability, extract GetCachedOfflineProbeResult() so GetUvxDevFlags()/GetUvxDevFlagsList() evaluate ShouldForceUvxRefresh() only once per call instead of twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
511-516: Trailing spaces in returned flag strings are fragile.
GetUvxDevFlags(bool, bool)returns strings with trailing spaces ("--no-cache --refresh ","--offline "). This works when callers always concatenate more content, but breaks if the result is used as a standalone value (e.g., trimmed comparisons in tests, or as the final segment of a command). Consider returning without trailing spaces and letting callers handle joining, or at minimum add a doc note that the trailing space is intentional.Suggested approach
- public static string GetUvxDevFlags(bool forceRefresh, bool useOffline) - { - if (forceRefresh) return "--no-cache --refresh "; - if (useOffline) return "--offline "; - return string.Empty; - } + public static string GetUvxDevFlags(bool forceRefresh, bool useOffline) + { + if (forceRefresh) return "--no-cache --refresh"; + if (useOffline) return "--offline"; + return string.Empty; + }Callers that concatenate would use e.g.
$"{devFlags} {remainingArgs}".TrimStart()or check for empty before inserting a space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 511 - 516, GetUvxDevFlags(bool forceRefresh, bool useOffline) currently returns flag strings with trailing spaces which is fragile; change the method to return flags without trailing spaces (e.g. "--no-cache --refresh" and "--offline") and update callers to handle spacing when concatenating (e.g., insert a single space only when the returned string is non-empty or use string.Join/Trim when building command arguments); keep the method signature and name GetUvxDevFlags unchanged and add a brief comment that callers are responsible for joining with surrounding arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs`:
- Around line 511-516: GetUvxDevFlags(bool forceRefresh, bool useOffline)
currently returns flag strings with trailing spaces which is fragile; change the
method to return flags without trailing spaces (e.g. "--no-cache --refresh" and
"--offline") and update callers to handle spacing when concatenating (e.g.,
insert a single space only when the returned string is non-empty or use
string.Join/Trim when building command arguments); keep the method signature and
name GetUvxDevFlags unchanged and add a brief comment that callers are
responsible for joining with surrounding arguments.
When HTTP transport is enabled, skip Configure() for clients that do not support HTTP (e.g., Claude Desktop). Previously this threw and logged a spurious warning on every editor launch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs (1)
77-81: Guard logic looks correct.This properly prevents the migration from attempting to configure clients that don't support the active HTTP transport, avoiding the exception path noted in the comment.
One minor nit:
EditorConfigurationCache.Instance.UseHttpTransportis loop-invariant — consider hoisting it above theforeachto make that clear.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs` around lines 77 - 81, Hoist the loop-invariant lookup of EditorConfigurationCache.Instance.UseHttpTransport out of the foreach: retrieve it once into a local bool (instead of calling EditorConfigurationCache.Instance.UseHttpTransport repeatedly) before iterating over the configurators in StdIoVersionMigration, then use that local (previously named useHttp) inside the loop to check configurator.Client.SupportsHttpTransport; this clarifies intent and avoids redundant property access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Migrations/StdIoVersionMigration.cs`:
- Around line 77-81: Hoist the loop-invariant lookup of
EditorConfigurationCache.Instance.UseHttpTransport out of the foreach: retrieve
it once into a local bool (instead of calling
EditorConfigurationCache.Instance.UseHttpTransport repeatedly) before iterating
over the configurators in StdIoVersionMigration, then use that local (previously
named useHttp) inside the loop to check
configurator.Client.SupportsHttpTransport; this clarifies intent and avoids
redundant property access.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--offlineto skip the network dependency check that can hang for 30+ seconds on slower/absent networks (international internet, corporate firewalls, etc)uvx --offline ... --help) with a 3-second timeout determines cache warmth before building the command--no-cache --refresh(current behavior)Changes
ShouldUseUvxOffline()method with cache warmth probe--offlinebranch inBuildUvxArgs()AddDevModeArgs→AddUvxModeFlags, added offline branch--offlinesupport inRegister(),RegisterWithCapturedValues(),GetManualSnippet()shouldUseOfflineon main thread for async configTest plan
--offlinepresent in generated commanduv cache clean):--offlineabsent, normal network resolution--no-cache --refreshused (not--offline)Closes #760
🤖 Generated with Claude Code
Summary by Sourcery
Add conditional use of uvx --offline mode for Unity MCP server launches to improve startup performance when packages are already cached while preserving existing behavior for refreshes and first-time installs.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Changes
Tests