-
Notifications
You must be signed in to change notification settings - Fork 459
Fix/winstore uv detection #253
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
Conversation
…ous ImportAsset and RequestScriptCompilation\n- Debounced refresh calls ImportAsset instead of Refresh\n- Path sanitation for Assets/ and slashes\n- Atomic writes with Windows-safe fallbacks\n- Nudge Editor tick for debounce reliability
WalkthroughAdds a Windows Store Python probe to FindUvPath on Windows, expands asset import scheduling to sanitize paths, nudges the editor loop via QueuePlayerLoopUpdate, and bumps the server version to 3.2.0. Public API adds ManageScriptRefreshHelpers.SanitizeAssetsPath(string). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Editor as Unity Editor
participant ServerInstaller as ServerInstaller.FindUvPath
participant OS as Windows FS/Env
User->>Editor: Trigger uv path resolution
Editor->>ServerInstaller: FindUvPath()
ServerInstaller->>OS: Check PATH / where uv.exe
alt uv.exe found on PATH
ServerInstaller-->>Editor: Return uv.exe path
else Not on PATH
note over ServerInstaller: New probe
ServerInstaller->>OS: Scan %LOCALAPPDATA%/Packages/PythonSoftwareFoundation.Python.*
ServerInstaller->>OS: Traverse LocalCache/local-packages/Python*/Scripts/uv.exe
alt uv.exe validated
ServerInstaller-->>Editor: Return uv.exe path (Windows Store)
else Not found / error
ServerInstaller->>OS: Fallback to existing candidate resolution
ServerInstaller-->>Editor: Return candidate or null
end
end
sequenceDiagram
autonumber
actor Source as Change Source
participant Manage as ManageScript (Scheduler)
participant Helpers as ManageScriptRefreshHelpers
participant Unity as AssetDatabase/Editor
Source->>Manage: ScheduleScriptRefresh(relPath)
Manage->>Helpers: SanitizeAssetsPath(relPath) Note over Helpers: Normalize slashes, strip prefixes, ensure "Assets/"
Helpers-->>Manage: sanitizedPath
Manage->>Unity: Queue import of sanitizedPath
Manage->>Unity: EditorApplication.QueuePlayerLoopUpdate() Note over Unity: Nudge loop for unfocused editor
loop Tick
Manage->>Helpers: SanitizeAssetsPath(p)
Helpers-->>Manage: sp
Manage->>Unity: ImportAsset(sp, ForceUpdate|ForceSynchronousImport)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
573-596
: Nice addition: Windows Store (Appx) Python probe for uv.exe.Good coverage of PythonSoftwareFoundation package layouts and safe try/catch wrapping. One reliability tweak: PATH entries can sometimes be quoted; consider trimming quotes in the later manual PATH scan to avoid false negatives.
Apply this to the manual PATH scan to handle quoted segments:
- string[] parts = pathEnv.Split(Path.PathSeparator); - foreach (string part in parts) + string[] parts = pathEnv.Split(Path.PathSeparator); + foreach (string part in parts) { try { - // Check both uv and uv.exe - string candidateUv = Path.Combine(part, "uv"); - string candidateUvExe = Path.Combine(part, "uv.exe"); + // Check both uv and uv.exe; trim surrounding quotes on PATH entries + var dir = (part ?? string.Empty).Trim().Trim('"'); + if (string.IsNullOrWhiteSpace(dir)) continue; + string candidateUv = Path.Combine(dir, "uv"); + string candidateUvExe = Path.Combine(dir, "uv.exe"); if (File.Exists(candidateUv) && ValidateUvBinary(candidateUv)) return candidateUv; if (File.Exists(candidateUvExe) && ValidateUvBinary(candidateUvExe)) return candidateUvExe; } catch { } }UnityMcpBridge/Editor/Tools/ManageScript.cs (4)
2571-2573
: Guard QueuePlayerLoopUpdate by Unity version to avoid compile breaks on older editors.Safe no-op on older versions; keeps the “nudge” on supported versions.
Apply:
- // Nudge the editor loop so ticks run even if the window is unfocused - EditorApplication.QueuePlayerLoopUpdate(); + // Nudge the editor loop so ticks run even if the window is unfocused +#if UNITY_2020_2_OR_NEWER + EditorApplication.QueuePlayerLoopUpdate(); +#endif
2600-2604
: Sanitizing before import is good; add a null/empty guard to be defensive.Prevents accidental null path imports if a caller ever passes empty.
- foreach (var p in toImport) - { - var sp = ManageScriptRefreshHelpers.SanitizeAssetsPath(p); - AssetDatabase.ImportAsset(sp, ImportAssetOptions.ForceUpdate | ImportAssetOptions.ForceSynchronousImport); - } + foreach (var p in toImport) + { + var sp = ManageScriptRefreshHelpers.SanitizeAssetsPath(p); + if (string.IsNullOrEmpty(sp)) continue; + AssetDatabase.ImportAsset(sp, ImportAssetOptions.ForceUpdate | ImportAssetOptions.ForceSynchronousImport); + }
2615-2626
: Scope the method to internal for clarity.The containing type isn’t public; making the method public suggests an API surface that isn’t actually exposed. Prefer internal for consistency.
- public static string SanitizeAssetsPath(string p) + internal static string SanitizeAssetsPath(string p)
2636-2640
: Defensive guard before ImportAsset.Mirror the Tick() check to avoid null path imports if called directly.
- var sp = SanitizeAssetsPath(relPath); + var sp = SanitizeAssetsPath(relPath); + if (string.IsNullOrEmpty(sp)) return; var opts = ImportAssetOptions.ForceUpdate; if (synchronous) opts |= ImportAssetOptions.ForceSynchronousImport; AssetDatabase.ImportAsset(sp, opts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs
(2 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs
(3 hunks)UnityMcpBridge/UnityMcpServer~/src/server_version.txt
(1 hunks)
🔇 Additional comments (3)
UnityMcpBridge/UnityMcpServer~/src/server_version.txt (1)
1-1
: Version bump acknowledged (3.2.0).Matches the installer’s semver compare flow; no further action.
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (1)
6-6
: LINQ import looks necessary.Required for OrderByDescending in the Windows Store probe.
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
2630-2632
: LGTM: Centralizing sanitize at scheduling call site.Reduces downstream surprises and normalizes inputs early.
Summary by CodeRabbit