Conversation
This reverts commit a0a2ad9.
📝 WalkthroughWalkthroughThis PR introduces widespread networking upgrades by migrating HTTP calls from UnityWebRequest to HttpClient across editor services, adds YouTube video metadata support with new serializable data models, refactors message flow timing in runtime components, updates avatar management UI visibility, and implements a package uninstaller command. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
Editor/SDKInitialiseOnLoad.cs (1)
39-39: Remove the commented-out uninstaller scaffold.The real uninstall flow now lives in
Editor/Resources/Builder/MenuUtils.cs. Keeping a second, fully commented copy here has already started to drift (pacakge.json, different cleanup steps) and makes the initializer much harder to scan.Also applies to: 73-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/SDKInitialiseOnLoad.cs` at line 39, Remove the commented-out uninstaller scaffold from Editor/SDKInitialiseOnLoad.cs: delete the commented line "// CreateUninstaller();" and the larger commented block spanning the existing uninstaller scaffold (previously lines 73-156) so the initializer no longer contains the outdated, duplicated uninstall logic; rely on the real uninstall flow in Editor/Resources/Builder/MenuUtils.cs and ensure no other commented duplicate functions (e.g., CreateUninstaller) remain in this file.Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs (2)
719-759:HttpRequestMessageandHttpResponseMessagenot disposed.Same resource management concern as other methods. Both objects should be disposed after use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 719 - 759, JsonGet currently creates an HttpRequestMessage (request) and obtains an HttpResponseMessage (response) without disposing them; wrap request creation and the code path that uses response in proper disposal (e.g., using blocks or try/finally Dispose calls) so request is disposed after SendAsync and response (and its Content if needed) is disposed after reading the content or on all error/early-return paths; update symbols referenced (JsonGet, request, _httpClient.SendAsync, response, response.Content, readTask) to ensure every exit/exception path disposes those objects.
687-717:HttpResponseMessageandStringContentnot disposed.Both the content and response should be disposed. The
StringContentpassed toPostAsyncand the returnedHttpResponseMessageimplementIDisposable.Suggested pattern
private IEnumerator PostFormEncodedStringNoAuth<T>(string urlPath, string data, Action<T> OnCompleted, Action<Exception> OnError) { - var content = new StringContent(data, Encoding.UTF8, "application/x-www-form-urlencoded"); - var task = _httpClient.PostAsync(new Uri(Config.RootApiUri, urlPath), content); + using var content = new StringContent(data, Encoding.UTF8, "application/x-www-form-urlencoded"); + var task = _httpClient.PostAsync(new Uri(Config.RootApiUri, urlPath), content); while (!task.IsCompleted) yield return null; // ... error handling ... - var response = task.Result; + using var response = task.Result; // ... rest of method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs` around lines 687 - 717, In PostFormEncodedStringNoAuth, both the created StringContent (content) and the HttpResponseMessage (response from _httpClient.PostAsync) must be disposed; wrap the StringContent creation in a using (or try/finally) and similarly dispose the response after you finish using it (i.e. after awaiting/finishing response.Content.ReadAsStringAsync and storing the result) to avoid leaking resources; ensure you still handle task faults, non-success status codes, and call OnError/OnCompleted as before while disposing in all control paths (use using/try-finally around content and a using/try-finally around response/readTask/result).Editor/Resources/Builder/BuilderWindow.cs (3)
1189-1201:HttpResponseMessagenot disposed.The response from
GetAsyncshould be disposed to release underlying resources (connection back to pool). Without disposal, connections may not be returned to the pool promptly.Suggested fix using try-finally
public IEnumerator Json<T>(string url, Action<T> callback) { var task = _httpClient.GetAsync(url); while (!task.IsCompleted) yield return null; if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; var response = task.Result; - if (!response.IsSuccessStatusCode) - throw new System.Exception(response.StatusCode + ": " + response.ReasonPhrase); - var readTask = response.Content.ReadAsStringAsync(); - while (!readTask.IsCompleted) yield return null; - if (readTask.IsFaulted) throw readTask.Exception.InnerException ?? readTask.Exception; - callback(JsonUtility.FromJson<T>(readTask.Result)); + try + { + if (!response.IsSuccessStatusCode) + throw new System.Exception(response.StatusCode + ": " + response.ReasonPhrase); + var readTask = response.Content.ReadAsStringAsync(); + while (!readTask.IsCompleted) yield return null; + if (readTask.IsFaulted) throw readTask.Exception.InnerException ?? readTask.Exception; + callback(JsonUtility.FromJson<T>(readTask.Result)); + } + finally + { + response.Dispose(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 1189 - 1201, The HttpResponseMessage returned in Json<T>(string url, Action<T> callback) is never disposed; wrap the code that inspects response and reads its content so the response is always disposed (e.g., assign response = task.Result, start the readTask, await its completion, and then in a try/finally or using-equivalent ensure response.Dispose() in the finally block). Make sure you read response.Content.ReadAsStringAsync() and obtain readTask.Result before disposing the response, and preserve existing error handling (throw on non-success status or task faults).
1203-1231:HttpRequestMessageandHttpResponseMessagenot disposed.Both the request and response objects implement
IDisposableand should be disposed to properly release resources. This is especially important for theStringContentwithin the request.Suggested fix pattern
public IEnumerator Json<T>(string url, T postData, Action<string> callback, Dictionary<string, string> headers = null) { var json = JsonUtility.ToJson(postData); - var request = new HttpRequestMessage(HttpMethod.Post, url) - { - Content = new StringContent(json, Encoding.UTF8, "application/json") - }; + using var request = new HttpRequestMessage(HttpMethod.Post, url) + { + Content = new StringContent(json, Encoding.UTF8, "application/json") + }; if (headers != null) { foreach (var header in headers) { if (!header.Key.Equals("Content-Type", StringComparison.OrdinalIgnoreCase)) request.Headers.TryAddWithoutValidation(header.Key, header.Value); } } var task = _httpClient.SendAsync(request); while (!task.IsCompleted) yield return null; if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; - var response = task.Result; + using var response = task.Result; // ... rest of the methodNote:
usingdeclarations require C# 8.0+. If using an older version, wrap in try-finally blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 1203 - 1231, The Json<T> coroutine creates an HttpRequestMessage (and a StringContent) and uses HttpResponseMessage from _httpClient.SendAsync without disposing them; wrap the HttpRequestMessage (and its Content) in a using (or try/finally Dispose) and also ensure the HttpResponseMessage is disposed after you finish reading its content (read response.Content.ReadAsStringAsync() and await/complete that before disposing); update the code around HttpRequestMessage creation, _httpClient.SendAsync call, and handling of response/response.Content (symbols: Json<T>, HttpRequestMessage, StringContent, _httpClient.SendAsync, HttpResponseMessage, response.Content.ReadAsStringAsync) so all IDisposable objects are properly disposed while preserving the existing error/exception handling and callback behavior.
53-54: Consider configuring timeout for consistency.The
HttpClientinstance here lacks timeout configuration, whileSqEditorAppApi.cssets a 10-minute timeout. For image downloads and API calls, consider adding a similar timeout configuration or using a sharedHttpClientinstance across both files to ensure consistent behavior.Suggested configuration
- private static readonly HttpClient _httpClient = new HttpClient(); + private static readonly HttpClient _httpClient = new HttpClient() + { + Timeout = TimeSpan.FromMinutes(5) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 53 - 54, The static HttpClient _httpClient in BuilderWindow.cs has no timeout set; update it to use the same 10-minute timeout as SqEditorAppApi.cs (or refactor to reuse the shared HttpClient used by SqEditorAppApi) by configuring the Timeout property on the shared/static HttpClient instance (refer to the _httpClient field in BuilderWindow and the HttpClient usage in SqEditorAppApi.cs) so both image downloads and API calls have consistent timeout behavior.Editor/Scripts/VisualScripting/VsStubsAllowed.cs (1)
12635-12798: Consider centralizing the 29 inherited Unity members shared across these four NavMesh types.This block duplicates the same inherited Object/Component surface (29 members) across NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface. If the allowlist matcher supports it, extracting these common members into a reusable list or type-level pattern would reduce the maintenance burden if Unity adds or renames inherited members in future releases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Scripts/VisualScripting/VsStubsAllowed.cs` around lines 12635 - 12798, The four types Unity.AI.Navigation.NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface repeat the same 29 inherited Object/Component members; extract those shared strings into a single reusable list (e.g., commonUnityComponentMembers) and reuse/concatenate it when building the allowlist entries for each of the four types (replace the duplicated block in the NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface entries with a reference to that shared list or a type-level pattern), so future changes to inherited Unity members only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Editor/Resources/Builder/BuilderWindow.cs`:
- Around line 376-384: In the Texture coroutine, check the boolean return value
of Texture2D.LoadImage after creating tex and loading task.Result; if LoadImage
returns false, dispose/destroy the invalid Texture2D, log or surface the error,
and invoke the provided callback with null (or throw a descriptive exception)
instead of passing an invalid texture; update the Texture method (the coroutine
named Texture and its callback usage) to handle the failed load path cleanly so
callers can detect and handle image decoding failures.
In `@Editor/Resources/Builder/MenuUtils.cs`:
- Around line 50-68: UninstallBanter() currently deletes the local package
folders but leaves file: entries in Packages/manifest.json, causing broken
references; update UninstallBanter() to load Packages/manifest.json, remove the
dependencies for "com.sidequest.ora" and the "com.basis.bundlemanagement",
"com.basis.sdk", "com.basis.odinserializer" keys (the same packages added by
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages()), write the
manifest back (preserving JSON formatting), then remove the directories and keep
the existing EditUtils.RemoveCompileDefine calls and
EditorApplication.OpenProject(Directory.GetCurrentDirectory()) so the project is
reopened with a cleaned manifest.
- Around line 45-49: The busy-wait loop that polls RemoveRequest.IsCompleted
after calling Client.Remove("com.sidequest.banter") blocks the editor update
pump; instead register a callback on EditorApplication.update to poll the
RemoveRequest until IsCompleted, then unregister the callback and continue
processing. Locate the call site using Client.Remove and the RemoveRequest
variable name, replace the while(!request.IsCompleted) { } spin with an
EditorApplication.update delegate that checks request.IsCompleted each tick,
calls whatever follow-up logic is needed when completed, and removes itself from
EditorApplication.update to avoid leaking the callback.
In `@Runtime/Scripts/BanterLink/BanterLink.cs`:
- Around line 147-153: Remove the hard-coded await new WaitForSeconds(2) and
instead rely on the concrete readiness check (do not introduce arbitrary
sleeps); use the existing WaitUntil that checks scene.loaded (and call
scene.SetLoaded() at the appropriate time before/inside that check) so emission
of UNITY_LOADED happens immediately when scene.loaded becomes true—modify the
block around WaitForSeconds/WaitUntil (references: WaitForSeconds, WaitUntil,
scene.SetLoaded(), scene.loaded, LogLine.Do) to await the readiness condition
only and remove the two-second delay.
- Around line 136-141: Restore handling for APICommands.DOM_READY by re-enabling
the branch that sets scene.state = SceneState.DOM_READY, invokes
scene.events.OnDomReady, and calls scene.SetLoaded() so DOM readiness
contributes to scene.isReady; locate the commented block around
APICommands.DOM_READY in BanterLink.cs and uncomment or reimplement that exact
behavior. If instead DOM_READY is intended to be retired, update the readiness
contract in the same change by modifying BanterScene.isReady and any listeners
that depend on OnDomReady to use the new readiness signal (ensure all references
to SceneState.DOM_READY, scene.events.OnDomReady.Invoke(), and scene.SetLoaded()
are removed or replaced consistently).
---
Nitpick comments:
In `@Editor/Resources/Builder/BuilderWindow.cs`:
- Around line 1189-1201: The HttpResponseMessage returned in Json<T>(string url,
Action<T> callback) is never disposed; wrap the code that inspects response and
reads its content so the response is always disposed (e.g., assign response =
task.Result, start the readTask, await its completion, and then in a try/finally
or using-equivalent ensure response.Dispose() in the finally block). Make sure
you read response.Content.ReadAsStringAsync() and obtain readTask.Result before
disposing the response, and preserve existing error handling (throw on
non-success status or task faults).
- Around line 1203-1231: The Json<T> coroutine creates an HttpRequestMessage
(and a StringContent) and uses HttpResponseMessage from _httpClient.SendAsync
without disposing them; wrap the HttpRequestMessage (and its Content) in a using
(or try/finally Dispose) and also ensure the HttpResponseMessage is disposed
after you finish reading its content (read response.Content.ReadAsStringAsync()
and await/complete that before disposing); update the code around
HttpRequestMessage creation, _httpClient.SendAsync call, and handling of
response/response.Content (symbols: Json<T>, HttpRequestMessage, StringContent,
_httpClient.SendAsync, HttpResponseMessage, response.Content.ReadAsStringAsync)
so all IDisposable objects are properly disposed while preserving the existing
error/exception handling and callback behavior.
- Around line 53-54: The static HttpClient _httpClient in BuilderWindow.cs has
no timeout set; update it to use the same 10-minute timeout as SqEditorAppApi.cs
(or refactor to reuse the shared HttpClient used by SqEditorAppApi) by
configuring the Timeout property on the shared/static HttpClient instance (refer
to the _httpClient field in BuilderWindow and the HttpClient usage in
SqEditorAppApi.cs) so both image downloads and API calls have consistent timeout
behavior.
In `@Editor/Resources/Builder/SideQuest/SqEditorAppApi.cs`:
- Around line 719-759: JsonGet currently creates an HttpRequestMessage (request)
and obtains an HttpResponseMessage (response) without disposing them; wrap
request creation and the code path that uses response in proper disposal (e.g.,
using blocks or try/finally Dispose calls) so request is disposed after
SendAsync and response (and its Content if needed) is disposed after reading the
content or on all error/early-return paths; update symbols referenced (JsonGet,
request, _httpClient.SendAsync, response, response.Content, readTask) to ensure
every exit/exception path disposes those objects.
- Around line 687-717: In PostFormEncodedStringNoAuth, both the created
StringContent (content) and the HttpResponseMessage (response from
_httpClient.PostAsync) must be disposed; wrap the StringContent creation in a
using (or try/finally) and similarly dispose the response after you finish using
it (i.e. after awaiting/finishing response.Content.ReadAsStringAsync and storing
the result) to avoid leaking resources; ensure you still handle task faults,
non-success status codes, and call OnError/OnCompleted as before while disposing
in all control paths (use using/try-finally around content and a
using/try-finally around response/readTask/result).
In `@Editor/Scripts/VisualScripting/VsStubsAllowed.cs`:
- Around line 12635-12798: The four types Unity.AI.Navigation.NavMeshLink,
NavMeshModifier, NavMeshModifierVolume, and NavMeshSurface repeat the same 29
inherited Object/Component members; extract those shared strings into a single
reusable list (e.g., commonUnityComponentMembers) and reuse/concatenate it when
building the allowlist entries for each of the four types (replace the
duplicated block in the NavMeshLink, NavMeshModifier, NavMeshModifierVolume, and
NavMeshSurface entries with a reference to that shared list or a type-level
pattern), so future changes to inherited Unity members only need to be made in
one place.
In `@Editor/SDKInitialiseOnLoad.cs`:
- Line 39: Remove the commented-out uninstaller scaffold from
Editor/SDKInitialiseOnLoad.cs: delete the commented line "//
CreateUninstaller();" and the larger commented block spanning the existing
uninstaller scaffold (previously lines 73-156) so the initializer no longer
contains the outdated, duplicated uninstall logic; rely on the real uninstall
flow in Editor/Resources/Builder/MenuUtils.cs and ensure no other commented
duplicate functions (e.g., CreateUninstaller) remain in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 884f27af-e9b8-4a0d-8331-e091c6515ac4
⛔ Files ignored due to path filters (1)
Runtime/Objects/Loading/LoadingCage.matis excluded by!**/*.mat
📒 Files selected for processing (18)
Editor/Resources/Builder/BuilderWindow.csEditor/Resources/Builder/BuilderWindow.uxmlEditor/Resources/Builder/MenuUtils.csEditor/Resources/Builder/SideQuest/SqEditorAppApi.csEditor/SDKInitialiseOnLoad.csEditor/Scripts/VisualScripting/VsStubsAllowed.csRuntime/Scripts/BanterLink/BanterLink.csRuntime/Scripts/BanterLink/BanterPipe.csRuntime/Scripts/BanterStarterUpper.csRuntime/Scripts/Models/YtInfo/YtResponseContext.csRuntime/Scripts/Models/YtInfo/YtThumbnail.csRuntime/Scripts/Models/YtInfo/YtThumbnail.cs.metaRuntime/Scripts/Models/YtInfo/YtVideoDetails.csRuntime/Scripts/Models/YtInfo/YtVideoDetails.cs.metaRuntime/Scripts/Scene/BanterScene.csRuntime/Scripts/Scene/Components/BanterBrowser.csRuntime/Scripts/Utils/AddPanelStuff.csRuntime/Scripts/Utils/DontDestroyOnLoad.cs
💤 Files with no reviewable changes (3)
- Runtime/Scripts/Utils/AddPanelStuff.cs
- Runtime/Scripts/BanterStarterUpper.cs
- Runtime/Scripts/BanterLink/BanterPipe.cs
Comment on lines
376
to
384
| public IEnumerator Texture(string url, Action<Texture2D> callback) | ||
| { | ||
| using (UnityWebRequest uwr = UnityWebRequestTexture.GetTexture(url)) | ||
| { | ||
| yield return uwr.SendWebRequest(); | ||
| if (uwr.result != UnityWebRequest.Result.Success) | ||
| { | ||
| throw new System.Exception(uwr.error); | ||
| } | ||
| else | ||
| { | ||
| callback(DownloadHandlerTexture.GetContent(uwr)); | ||
| } | ||
| } | ||
| var task = _httpClient.GetByteArrayAsync(url); | ||
| while (!task.IsCompleted) yield return null; | ||
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | ||
| var tex = new Texture2D(1, 1); | ||
| tex.LoadImage(task.Result); | ||
| callback(tex); | ||
| } |
There was a problem hiding this comment.
Unchecked LoadImage return value.
Texture2D.LoadImage() returns a boolean indicating success. If the downloaded bytes are not a valid image format, this will fail silently and the callback will receive an invalid texture.
Suggested fix
var tex = new Texture2D(1, 1);
- tex.LoadImage(task.Result);
- callback(tex);
+ if (!tex.LoadImage(task.Result))
+ {
+ UnityEngine.Object.DestroyImmediate(tex);
+ throw new Exception($"Failed to load image from URL: {url}");
+ }
+ callback(tex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public IEnumerator Texture(string url, Action<Texture2D> callback) | |
| { | |
| using (UnityWebRequest uwr = UnityWebRequestTexture.GetTexture(url)) | |
| { | |
| yield return uwr.SendWebRequest(); | |
| if (uwr.result != UnityWebRequest.Result.Success) | |
| { | |
| throw new System.Exception(uwr.error); | |
| } | |
| else | |
| { | |
| callback(DownloadHandlerTexture.GetContent(uwr)); | |
| } | |
| } | |
| var task = _httpClient.GetByteArrayAsync(url); | |
| while (!task.IsCompleted) yield return null; | |
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | |
| var tex = new Texture2D(1, 1); | |
| tex.LoadImage(task.Result); | |
| callback(tex); | |
| } | |
| public IEnumerator Texture(string url, Action<Texture2D> callback) | |
| { | |
| var task = _httpClient.GetByteArrayAsync(url); | |
| while (!task.IsCompleted) yield return null; | |
| if (task.IsFaulted) throw task.Exception.InnerException ?? task.Exception; | |
| var tex = new Texture2D(1, 1); | |
| if (!tex.LoadImage(task.Result)) | |
| { | |
| UnityEngine.Object.DestroyImmediate(tex); | |
| throw new Exception($"Failed to load image from URL: {url}"); | |
| } | |
| callback(tex); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/BuilderWindow.cs` around lines 376 - 384, In the
Texture coroutine, check the boolean return value of Texture2D.LoadImage after
creating tex and loading task.Result; if LoadImage returns false,
dispose/destroy the invalid Texture2D, log or surface the error, and invoke the
provided callback with null (or throw a descriptive exception) instead of
passing an invalid texture; update the Texture method (the coroutine named
Texture and its callback usage) to handle the failed load path cleanly so
callers can detect and handle image decoding failures.
Comment on lines
+45
to
+49
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | ||
| while(!request.IsCompleted) | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Editor/Resources/Builder/MenuUtils.cs | sed -n '40,60p'Repository: SideQuestVR/BanterSDK
Length of output: 1033
🏁 Script executed:
# Search for RemoveRequest usage in the codebase to understand the pattern
rg "RemoveRequest" -A 5 -B 2Repository: SideQuestVR/BanterSDK
Length of output: 1260
🌐 Web query:
UnityEditor.PackageManager.Client.Remove RemoveRequest async completion IsCompleted editor update
💡 Result:
Client.Remove(...) returns a RemoveRequest and runs asynchronously—there’s no callback/event on the request itself, so you detect completion by polling request.IsCompleted and then checking request.Status / request.Error. IsCompleted becomes true only when the UPM operation finishes. [1][2]
Typical Editor pattern (poll each editor tick): [3]
using UnityEditor;
using UnityEditor.PackageManager;
using UnityEditor.PackageManager.Requests;
static RemoveRequest _req;
static void RemovePkg(string name)
{
_req = Client.Remove(name);
EditorApplication.update += Tick;
}
static void Tick()
{
if (!_req.IsCompleted) return;
EditorApplication.update -= Tick;
if (_req.Status == StatusCode.Success) UnityEngine.Debug.Log("Removed");
else UnityEngine.Debug.LogError(_req.Error.message);
}If you also need to know when the Editor has applied/compiled/imported the resulting package set (not just when the request completed), subscribe to UnityEditor.PackageManager.Events.registeredPackages (raised after the Editor applies the changed list). [4] Unity notes cases where the change is only effective after a “full editor tick”, so chaining package operations is often done sequentially via EditorApplication.update (or by waiting for registeredPackages). [5]
Sources: [1] Client.Remove doc, [2] Request.IsCompleted/Status/Error, [3] Unity manual example showing the EditorApplication.update + IsCompleted polling pattern, [4] Package Manager Events, [5] Unity Issue Tracker note about changes becoming effective after a full editor tick.
Replace the busy-wait loop with EditorApplication.update callback.
Client.Remove() completes asynchronously during editor updates. The tight loop on lines 46–49 blocks that update pump, freezing the editor and preventing the uninstall from ever completing. Use EditorApplication.update to poll IsCompleted instead.
Safer pattern
RemoveRequest request = Client.Remove("com.sidequest.banter");
- while(!request.IsCompleted)
- {
-
- }
+ EditorApplication.update += WaitForRemove;
+ void WaitForRemove()
+ {
+ if (!request.IsCompleted) return;
+ EditorApplication.update -= WaitForRemove;
+ if (request.Status == StatusCode.Success)
+ {
+ // continue cleanup here
+ }
+ else
+ {
+ Debug.LogError($"Remove failed: {request.Error.message}");
+ }
+ }
+ return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | |
| while(!request.IsCompleted) | |
| { | |
| } | |
| RemoveRequest request = Client.Remove("com.sidequest.banter"); | |
| EditorApplication.update += WaitForRemove; | |
| void WaitForRemove() | |
| { | |
| if (!request.IsCompleted) return; | |
| EditorApplication.update -= WaitForRemove; | |
| if (request.Status == StatusCode.Success) | |
| { | |
| // continue cleanup here | |
| } | |
| else | |
| { | |
| Debug.LogError($"Remove failed: {request.Error.message}"); | |
| } | |
| } | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/MenuUtils.cs` around lines 45 - 49, The busy-wait
loop that polls RemoveRequest.IsCompleted after calling
Client.Remove("com.sidequest.banter") blocks the editor update pump; instead
register a callback on EditorApplication.update to poll the RemoveRequest until
IsCompleted, then unregister the callback and continue processing. Locate the
call site using Client.Remove and the RemoveRequest variable name, replace the
while(!request.IsCompleted) { } spin with an EditorApplication.update delegate
that checks request.IsCompleted each tick, calls whatever follow-up logic is
needed when completed, and removes itself from EditorApplication.update to avoid
leaking the callback.
Comment on lines
+50
to
+68
| if(Directory.Exists("Packages/com.basis.bundlemanagement")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.bundlemanagement", true); | ||
| } | ||
| if(Directory.Exists("Packages/com.basis.sdk")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.sdk", true); | ||
| } | ||
| if(Directory.Exists("Packages/com.basis.odinserializer")) | ||
| { | ||
| Directory.Delete("Packages/com.basis.odinserializer", true); | ||
| } | ||
| if (Directory.Exists("Packages/com.sidequest.ora")) | ||
| { | ||
| Directory.Delete("Packages/com.sidequest.ora", true); | ||
| } | ||
| EditUtils.RemoveCompileDefine("BANTER_ORA", new BuildTargetGroup[] { BuildTargetGroup.Android, BuildTargetGroup.Standalone }); | ||
| EditUtils.RemoveCompileDefine("BASIS_BUNDLE_MANAGEMENT", new BuildTargetGroup[] { BuildTargetGroup.Android, BuildTargetGroup.Standalone }); | ||
| EditorApplication.OpenProject(Directory.GetCurrentDirectory()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MenuUtils.cs" | head -20Repository: SideQuestVR/BanterSDK
Length of output: 104
🏁 Script executed:
find . -type f -name "manifest.json" | head -10Repository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
rg -l "ImportOraPackage\|ImportBasisPackages" --type csRepository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
cat -n "./Editor/Resources/Builder/MenuUtils.cs" | head -100Repository: SideQuestVR/BanterSDK
Length of output: 3531
🏁 Script executed:
rg "manifest\.json|ImportOraPackage|ImportBasisPackages|InitialiseOnLoad" --type cs -iRepository: SideQuestVR/BanterSDK
Length of output: 928
🏁 Script executed:
git ls-files | grep -i manifestRepository: SideQuestVR/BanterSDK
Length of output: 47
🏁 Script executed:
cat -n "./Editor/SDKInitialiseOnLoad.cs"Repository: SideQuestVR/BanterSDK
Length of output: 19671
Remove the manifest.json entries for the deleted packages.
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages() add com.sidequest.ora and the com.basis.* packages (com.basis.bundlemanagement, com.basis.sdk, com.basis.odinserializer) as file: dependencies to Packages/manifest.json. Deleting these directories in UninstallBanter() without removing those manifest entries leaves the reopened project with broken package references pointing to missing local packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Editor/Resources/Builder/MenuUtils.cs` around lines 50 - 68,
UninstallBanter() currently deletes the local package folders but leaves file:
entries in Packages/manifest.json, causing broken references; update
UninstallBanter() to load Packages/manifest.json, remove the dependencies for
"com.sidequest.ora" and the "com.basis.bundlemanagement", "com.basis.sdk",
"com.basis.odinserializer" keys (the same packages added by
InitialiseOnLoad.ImportOraPackage() and ImportBasisPackages()), write the
manifest back (preserving JSON formatting), then remove the directories and keep
the existing EditUtils.RemoveCompileDefine calls and
EditorApplication.OpenProject(Directory.GetCurrentDirectory()) so the project is
reopened with a cleaned manifest.
Comment on lines
+136
to
+141
| // else if (msg.StartsWith(APICommands.DOM_READY)) | ||
| // { | ||
| // scene.state = SceneState.DOM_READY; | ||
| // scene.events.OnDomReady.Invoke(); | ||
| // scene.SetLoaded(); | ||
| // } |
There was a problem hiding this comment.
Restore DOM_READY or update the readiness contract in the same change.
This was the only branch that raised scene.events.OnDomReady and transitioned the scene into SceneState.DOM_READY. With it commented out, that callback never fires, and DOM readiness no longer participates in the scene.isReady progression. If DOM_READY is being retired, BanterScene.isReady and any listeners should be updated in the same PR.
💡 Minimal fix
- // else if (msg.StartsWith(APICommands.DOM_READY))
- // {
- // scene.state = SceneState.DOM_READY;
- // scene.events.OnDomReady.Invoke();
- // scene.SetLoaded();
- // }
+ else if (msg.StartsWith(APICommands.DOM_READY))
+ {
+ scene.state = SceneState.DOM_READY;
+ scene.events.OnDomReady.Invoke();
+ scene.SetLoaded();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // else if (msg.StartsWith(APICommands.DOM_READY)) | |
| // { | |
| // scene.state = SceneState.DOM_READY; | |
| // scene.events.OnDomReady.Invoke(); | |
| // scene.SetLoaded(); | |
| // } | |
| else if (msg.StartsWith(APICommands.DOM_READY)) | |
| { | |
| scene.state = SceneState.DOM_READY; | |
| scene.events.OnDomReady.Invoke(); | |
| scene.SetLoaded(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/Scripts/BanterLink/BanterLink.cs` around lines 136 - 141, Restore
handling for APICommands.DOM_READY by re-enabling the branch that sets
scene.state = SceneState.DOM_READY, invokes scene.events.OnDomReady, and calls
scene.SetLoaded() so DOM readiness contributes to scene.isReady; locate the
commented block around APICommands.DOM_READY in BanterLink.cs and uncomment or
reimplement that exact behavior. If instead DOM_READY is intended to be retired,
update the readiness contract in the same change by modifying
BanterScene.isReady and any listeners that depend on OnDomReady to use the new
readiness signal (ensure all references to SceneState.DOM_READY,
scene.events.OnDomReady.Invoke(), and scene.SetLoaded() are removed or replaced
consistently).
Comment on lines
+147
to
+153
| await new WaitForSeconds(2); | ||
| await new WaitUntil(() => | ||
| { | ||
| scene.SetLoaded(); | ||
| return scene.loaded; | ||
| }); | ||
| }); | ||
| LogLine.Do(LogLine.banterColor, LogTag.Banter, "After Banter Scene Loaded."); |
There was a problem hiding this comment.
Avoid a hard-coded 2s pause in the load handshake.
Line 147 adds a guaranteed two-second delay before UNITY_LOADED can be emitted, even when the scene is already ready. If this is masking another race, please wait on that concrete condition instead of slowing every load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/Scripts/BanterLink/BanterLink.cs` around lines 147 - 153, Remove the
hard-coded await new WaitForSeconds(2) and instead rely on the concrete
readiness check (do not introduce arbitrary sleeps); use the existing WaitUntil
that checks scene.loaded (and call scene.SetLoaded() at the appropriate time
before/inside that check) so emission of UNITY_LOADED happens immediately when
scene.loaded becomes true—modify the block around WaitForSeconds/WaitUntil
(references: WaitForSeconds, WaitUntil, scene.SetLoaded(), scene.loaded,
LogLine.Do) to await the readiness condition only and remove the two-second
delay.
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 by CodeRabbit
New Features
Improvements