perf: avoid blocking update checks in editor window#954
Open
jiajunfeng wants to merge 1 commit intoCoplayDev:betafrom
Open
perf: avoid blocking update checks in editor window#954jiajunfeng wants to merge 1 commit intoCoplayDev:betafrom
jiajunfeng wants to merge 1 commit 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
📝 WalkthroughWalkthroughThe pull request refactors HTTP client creation in Changes
Sequence DiagramsequenceDiagram
participant UI as EditorWindow UI
participant Gate as Update Check Gate
participant Task as Task Executor
participant Service as PackageUpdateService
participant Web as TimeoutWebClient
UI->>Gate: QueueUpdateCheck()
alt updateCheckQueued or updateCheckInFlight?
Gate-->>UI: Return (blocked)
else Proceed
Gate->>Gate: Set updateCheckQueued/inFlight = true
Gate->>Task: Task.Run(CheckForPackageUpdates)
Task->>Service: CheckForUpdate()
Service->>Service: FetchLatestVersionFromGitHub/AssetStore
Service->>Web: CreateWebClient() + Execute Request
Web->>Web: Override GetWebRequest (set Timeout)
Web-->>Service: Response
Service-->>Task: Update result
Task->>UI: EditorApplication.delayCall
UI->>UI: Update UI (updateNotification)
UI->>Gate: Clear updateCheckInFlight
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![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
Comment on lines
+382
to
+387
| Task.Run(() => | ||
| { | ||
| try | ||
| { | ||
| return MCPServiceLocator.Updates.CheckForUpdate(currentVersion); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the background-thread call path and UnityEditor API usage.
rg -n "Task.Run|CheckForPackageUpdates|CheckForUpdate\\(" MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs -C 3
rg -n "EditorPrefs\\.(GetString|SetString|DeleteKey)" MCPForUnity/Editor/Services/PackageUpdateService.cs -C 2
rg -n "public static IPackageUpdateService Updates =>" MCPForUnity/Editor/Services/MCPServiceLocator.cs -C 2Repository: CoplayDev/unity-mcp
Length of output: 2267
🏁 Script executed:
# Get the full implementation of CheckForUpdate to see all code paths
rg -n "public.*CheckForUpdate" MCPForUnity/Editor/Services/PackageUpdateService.cs -A 50Repository: CoplayDev/unity-mcp
Length of output: 2529
🏁 Script executed:
# Check if there's any thread-safety or main-thread queuing already in place
rg -n "EditorApplication\.(delayCall|update)" MCPForUnity/Editor/Services/PackageUpdateService.csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Verify the full context of the Task.Run usage in the editor window
cat -n MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs | sed -n '365,400p'Repository: CoplayDev/unity-mcp
Length of output: 1525
Move EditorPrefs access to the editor thread; only run network I/O on background threads.
Task.Run at line 382 executes MCPServiceLocator.Updates.CheckForUpdate(...) on a worker thread. That method calls EditorPrefs.GetString(...) unconditionally at lines 42–43 (before any network I/O), which is not thread-safe. Additionally, the lazy initialization of MCPServiceLocator.Updates at line 29 happens on the worker thread on first access.
While the result processing correctly uses EditorApplication.delayCall to queue back to the main thread (line 395), the EditorPrefs calls have already executed unsafely. Refactor to read/write the editor cache on the main thread and move only the network fetch (FetchLatestVersionFromGitHub / FetchLatestVersionFromAssetStoreJson) to the background thread.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs` around lines 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.
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
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