perf: avoid blocking update checks in editor window#954
Merged
Scriptwonder merged 3 commits intoCoplayDev:betafrom Mar 23, 2026
Merged
perf: avoid blocking update checks in editor window#954Scriptwonder merged 3 commits intoCoplayDev:betafrom
Scriptwonder merged 3 commits intoCoplayDev:betafrom
Conversation
Contributor
Reviewer's GuideRuns package update checks for the Unity editor window on a background task instead of the main thread, adds a timeout-configured WebClient for GitHub/Asset Store version fetches, and guards against re-queuing update checks while one is already in flight, without changing visible update banner behavior when results are available. Sequence diagram for asynchronous package update check in editor windowsequenceDiagram
actor User
participant MCPForUnityEditorWindow
participant EditorApplication
participant TaskScheduler
participant PackageUpdateService
User->>MCPForUnityEditorWindow: Trigger QueueUpdateCheck
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: QueueUpdateCheck()
alt updateCheckQueued or updateCheckInFlight is true
MCPForUnityEditorWindow-->>User: Return (no new check queued)
else
MCPForUnityEditorWindow->>EditorApplication: delayCall += CheckForPackageUpdates
end
EditorApplication->>MCPForUnityEditorWindow: Invoke CheckForPackageUpdates
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: updateCheckQueued = false
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Get currentVersion
alt currentVersion invalid
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Hide updateNotification
MCPForUnityEditorWindow-->>User: No update check
else
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: updateCheckInFlight = true
MCPForUnityEditorWindow->>TaskScheduler: Task.Run(CheckForUpdate)
TaskScheduler->>PackageUpdateService: CheckForUpdate(currentVersion)
PackageUpdateService-->>TaskScheduler: UpdateCheckResult or null
TaskScheduler->>EditorApplication: ContinueWith -> delayCall callback
EditorApplication->>MCPForUnityEditorWindow: Invoke UI update callback
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: updateCheckInFlight = false
alt window or UI elements destroyed
MCPForUnityEditorWindow-->>User: Return (no UI update)
else
alt result indicates update available
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Set updateNotificationText
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Show updateNotification
else
MCPForUnityEditorWindow->>MCPForUnityEditorWindow: Hide updateNotification
end
end
end
Class diagram for PackageUpdateService and TimeoutWebClient changesclassDiagram
class MCPForUnityEditorWindow {
-bool toolsLoaded
-bool resourcesLoaded
-double lastRefreshTime
-const double RefreshDebounceSeconds
-bool updateCheckQueued
-bool updateCheckInFlight
+void QueueUpdateCheck()
+void CheckForPackageUpdates()
}
class PackageUpdateService {
-const int DefaultRequestTimeoutMs
-string LastCheckDateKey
-string CachedVersionKey
-string LastBetaCheckDateKey
+string FetchLatestVersionFromGitHub(string branch)
+string FetchLatestVersionFromAssetStoreJson()
+WebClient CreateWebClient()
+int GetRequestTimeoutMs()
}
class TimeoutWebClient {
-int _timeoutMs
+TimeoutWebClient(int timeoutMs)
+WebRequest GetWebRequest(Uri address)
}
MCPForUnityEditorWindow --> PackageUpdateService : uses
PackageUpdateService o-- TimeoutWebClient : creates
TimeoutWebClient --|> WebClient
Flow diagram for update check queuing and in-flight guardflowchart TD
A[QueueUpdateCheck called] --> B{updateCheckQueued<br/>or updateCheckInFlight}
B -- yes --> C[Return without queuing]
B -- no --> D[Set updateCheckQueued = true]
D --> E[EditorApplication.delayCall += CheckForPackageUpdates]
E --> F[CheckForPackageUpdates invoked]
F --> G[Set updateCheckQueued = false]
G --> H{currentVersion is null<br/>empty or unknown}
H -- yes --> I[Hide updateNotification]
I --> J[Return]
H -- no --> K[Set updateCheckInFlight = true]
K --> L["Task.Run(CheckForUpdate)"]
L --> M[Background CheckForUpdate completes]
M --> N[EditorApplication.delayCall UI callback]
N --> O[Set updateCheckInFlight = false]
O --> P{window or UI destroyed}
P -- yes --> Q[Return]
P -- no --> R{Update available<br/>and check succeeded}
R -- yes --> S[Set updateNotificationText<br/>and tooltip]
S --> T[Show updateNotification]
R -- no --> U[Hide updateNotification]
T --> V[End]
U --> V[End]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds per-day caching and background-safe fetch APIs to package update checks, centralizes WebClient/timeout creation, and updates the editor window to use cached results first and perform network fetches on a background task, marshaling UI updates back to the main thread. Changes
Sequence DiagramsequenceDiagram
actor User
participant EditorWindow as Editor Window
participant UpdateService as PackageUpdateService
participant WebClient as TimeoutWebClient
participant Prefs as EditorPrefs
User->>EditorWindow: QueueUpdateCheck()
EditorWindow->>EditorWindow: if cached today -> TryGetCachedResult(currentVersion)
alt cached result exists
EditorWindow->>EditorWindow: ApplyUpdateCheckResult(cached)
else no cached result
EditorWindow->>EditorWindow: start Task.Run -> FetchAndCompare(currentVersion)
UpdateService->>WebClient: CreateWebClient() / Fetch latest (GitHub or AssetStore)
WebClient->>UpdateService: HTTP response (version or error)
UpdateService->>UpdateService: compare versions -> UpdateCheckResult
UpdateService-->>EditorWindow: result (via Task continuation)
EditorWindow->>Prefs: CacheFetchResult(currentVersion, fetchedVersion) rgba(0,128,0,0.5)
EditorWindow->>EditorWindow: ApplyUpdateCheckResult(fetched)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 left some high level feedback:
- In
CheckForPackageUpdates,updateCheckInFlightis only reset inside the delayed UI callback and after thethis/updateNotificationnull checks, so if the window is closed or the notification elements are null, the method returns early and leavesupdateCheckInFlightstucktrue, preventing future update checks; move the reset before those early returns (or ensure it's done in afinally-style path) so the flag is always cleared.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CheckForPackageUpdates`, `updateCheckInFlight` is only reset inside the delayed UI callback and after the `this/updateNotification` null checks, so if the window is closed or the notification elements are null, the method returns early and leaves `updateCheckInFlight` stuck `true`, preventing future update checks; move the reset before those early returns (or ensure it's done in a `finally`-style path) so the flag is always cleared.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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs`:
- Around line 382-387: The CheckForUpdate call currently runs on a worker thread
and therefore performs unsafe EditorPrefs access and lazy initialization of
MCPServiceLocator.Updates off the main thread; instead, ensure
MCPServiceLocator.Updates is accessed on the editor thread and read any
EditorPrefs cache (EditorPrefs.GetString/SetString) on the main thread before
starting background work, then call Task.Run only to execute the network methods
(FetchLatestVersionFromGitHub / FetchLatestVersionFromAssetStoreJson) using the
cached values as parameters; finally, marshal the result back with
EditorApplication.delayCall and perform any EditorPrefs writes or
MCPServiceLocator.Updates lazy initialization there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 446a5176-1e3f-446d-885d-6673965b9e4a
📒 Files selected for processing (2)
MCPForUnity/Editor/Services/PackageUpdateService.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
Address CodeRabbit review feedback on PR CoplayDev#954: CheckForUpdate was running entirely inside Task.Run, including EditorPrefs reads/writes and MCPServiceLocator.Updates lazy initialization — both unsafe off the main thread. Split CheckForUpdate into three thread-aware methods: - TryGetCachedResult: reads EditorPrefs cache (main thread) - FetchAndCompare: network I/O only (background thread safe) - CacheFetchResult: writes EditorPrefs cache (main thread) The editor window now resolves the service and checks cache on the main thread, runs only network I/O in Task.Run, and caches results back on the main thread via EditorApplication.delayCall.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Services/IPackageUpdateService.cs`:
- Around line 15-19: Update the TryGetCachedResult contract to remove the
incorrect "Safe to call from any thread" claim and instead document that it must
be called on the Unity main thread because it reads EditorPrefs; change the XML
summary to state "Main-thread only" or equivalent. Also make the return type
nullable to match the implementation and comments: change the signature from
UpdateCheckResult TryGetCachedResult(string currentVersion) to
UpdateCheckResult? TryGetCachedResult(string currentVersion). Reference:
TryGetCachedResult, UpdateCheckResult, and EditorPrefs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5580160-5200-498e-bc3f-f2d667df63cc
📒 Files selected for processing (3)
MCPForUnity/Editor/Services/IPackageUpdateService.csMCPForUnity/Editor/Services/PackageUpdateService.csMCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs
- MCPForUnity/Editor/Services/PackageUpdateService.cs
Comment on lines
+15
to
+19
| /// <summary> | ||
| /// Returns a cached update result if one exists for today, or null if a network fetch is needed. | ||
| /// Safe to call from any thread context but designed for main-thread use (reads EditorPrefs). | ||
| /// </summary> | ||
| UpdateCheckResult TryGetCachedResult(string currentVersion); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is UnityEditor.EditorPrefs documented as safe to access from background threads, or should it be treated as main-thread-only in Unity Editor code?
💡 Result:
UnityEditor.EditorPrefs is not documented as safe to access from background threads. It should be treated as main-thread-only in Unity Editor code. Official Unity documentation for EditorPrefs (across multiple versions, including 2023.1 and current as of 2026) makes no mention of thread safety or background thread usage. All examples show usage in main-thread contexts like EditorWindow.OnGUI methods. Unity's general threading guidelines state that nearly all UnityEngine and UnityEditor APIs are not thread-safe unless explicitly documented otherwise (e.g., NativeContainers for jobs). Editor APIs, being stateful and tied to editor UI/operations, follow this rule. Forum discussions confirm EditorPrefs fails silently or behaves unexpectedly when called from callbacks like server responses (likely background/async), and community consensus lists it implicitly as main-thread-only alongside other Unity APIs. No sources document EditorPrefs as thread-safe, and Unity's pattern requires explicit main-thread dispatchers (e.g., Search.Dispatcher) for other editor APIs when needed.
Citations:
- 1: https://docs.unity3d.com/2023.1/Documentation/ScriptReference/EditorPrefs.html
- 2: https://docs.unity3d.com/ScriptReference/EditorPrefs.html
- 3: https://docs.unity3d.com/Documentation/ScriptReference/UnityEditor.html
- 4: https://discussions.unity.com/t/list-of-thread-safe-unity-calls/656846
- 5: https://discussions.unity.com/t/background-thread-supported-functionality/944855
- 6: https://discussions.unity.com/t/editorprefs-silently-fail-to-work-when-called-from-a-server-response-call-back/226106
- 7: https://discussions.unity.com/t/should-i-cache-editorprefs-or-can-i-write-to-them-every-frame/659715
🏁 Script executed:
# Find and read the IPackageUpdateService.cs file to see full implementation
fd "IPackageUpdateService.cs" --type fRepository: CoplayDev/unity-mcp
Length of output: 173
🏁 Script executed:
# Check project file for NRT (nullable reference types) settings
fd "\.csproj$|\.sln$" --type f | head -5Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Read the full interface file to see the actual signatures and implementation details
cat -n MCPForUnity/Editor/Services/IPackageUpdateService.csRepository: CoplayDev/unity-mcp
Length of output: 3515
🏁 Script executed:
# Check for csproj files more carefully
find . -name "*.csproj" -type f | head -5Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Check for csproj file in the repository
find . -type f -name "*.csproj" | head -10Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Also check for any assembly.cs files that might have NRT settings
find . -type f -name "AssemblyInfo.cs"Repository: CoplayDev/unity-mcp
Length of output: 192
🏁 Script executed:
# Check the AssemblyInfo.cs to see if NRT is enabled
cat -n ./MCPForUnity/Editor/AssemblyInfo.csRepository: CoplayDev/unity-mcp
Length of output: 183
🏁 Script executed:
# Also check for an .editorconfig or Directory.Build.props file that might configure NRT
find . -type f \( -name ".editorconfig" -o -name "Directory.Build.props" -o -name "csproj" \) | head -10Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Check the implementation file to see if TryGetCachedResult actually reads EditorPrefs
find . -type f -name "*PackageUpdateService.cs" | grep -v ".meta"Repository: CoplayDev/unity-mcp
Length of output: 171
🏁 Script executed:
# Read the implementation to confirm EditorPrefs usage
fd "PackageUpdateService\.cs" -t f --type f | grep -v ".meta" | grep -v "Interface"Repository: CoplayDev/unity-mcp
Length of output: 167
🏁 Script executed:
# Read the implementation file
cat -n MCPForUnity/Editor/Services/PackageUpdateService.csRepository: CoplayDev/unity-mcp
Length of output: 20656
Fix contradictory thread-safety contract in TryGetCachedResult.
Line 17 says this method is safe on any thread, but it reads EditorPrefs (lines 99–100 of the implementation), which Unity documents as main-thread-only. This direct contradiction can reintroduce off-main-thread bugs. Revise the contract to explicitly state main-thread-only usage.
Additionally, the method returns null (line 113) but the interface signature declares a non-nullable return type. If nullable reference types are enabled, update the signature to UpdateCheckResult? to match the documented behavior ("or null if a network fetch is needed").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Services/IPackageUpdateService.cs` around lines 15 - 19,
Update the TryGetCachedResult contract to remove the incorrect "Safe to call
from any thread" claim and instead document that it must be called on the Unity
main thread because it reads EditorPrefs; change the XML summary to state
"Main-thread only" or equivalent. Also make the return type nullable to match
the implementation and comments: change the signature from UpdateCheckResult
TryGetCachedResult(string currentVersion) to UpdateCheckResult?
TryGetCachedResult(string currentVersion). Reference: TryGetCachedResult,
UpdateCheckResult, and EditorPrefs.
- Remove incorrect "Safe to call from any thread" claim; method reads EditorPrefs which requires the Unity main thread - Add nullable return type annotation to match the null-on-cache-miss behavior
Collaborator
|
Thanks for the feedback! |
1 task
10 tasks
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
2 participants
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
Changes
CheckForUpdate(...)on a background task and marshal UI updates back withEditorApplication.delayCallWebClientwrapper to cap remote version fetch latency at 3 secondsValidation
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.csMCPForUnity/Editor/Services/PackageUpdateService.csSummary by Sourcery
Run Unity editor package update checks asynchronously with bounded network latency while preserving existing update banner behavior.
Bug Fixes:
Enhancements:
Summary by CodeRabbit