Unity client sdk #1105
Unity client sdk #1105
Conversation
Moved Unity SDK template files from 'templates/unity/Runtime' and 'templates/unity/Editor' to 'templates/unity/Assets/Runtime' and 'templates/unity/Assets/Editor' for better alignment with Unity project conventions. Updated getFiles() in Unity.php to reflect new paths and added support for copying plugin DLLs and project settings. Improved file upload logic in Client.cs.twig to handle streams and byte arrays more robustly, and removed Unity-specific logging from Exception.cs.twig. Minor fixes in Realtime.cs.twig and Role.cs.twig for namespace and async handling.
Introduces Unity2021 test support by adding a Unity2021Test.php, Unity test source files, and updating the GitHub Actions workflow to include Unity2021 in the test matrix and set the UNITY_LICENSE environment variable. This enables automated testing for the Unity SDK within the CI pipeline.
|
I am also attaching instructions on how to obtain a Unity license painlessly. |
|
The basic functionality tested in the test is guaranteed to work, but I'm not sure about the rest. I will continue to refine it. Also, a question: What to implement next for the client SDK? |
|
The test failed because the secret is not configured in appwrite/sdk-generator. The successful test is here: |
// Cookie support
client.SetCookie() / client.GetCookie()
// Real-time subscriptions
realtime.Subscribe(channels, callback)
// Connection testing
await client.Ping()The successful test is here: |
Add tests for new Query methods in Unity
The Unity SDK README was rewritten for clarity, modernized with improved badge links, updated installation and dependency instructions, and new quick start code samples for both AppwriteManager and direct Client usage
cosmetic fix
normalize domain value
The Unity language class now extends DotNet, removing Unity-specific overrides and template files. Unity code generation now uses shared .NET templates, reducing duplication and maintenance overhead. Updated file mappings and removed Unity-specific template files accordingly.
Refactor Unity SDK to inherit from DotNet and reuse templates
|
In any case, it will be necessary to accept #1138 first as it synchronized Unity and DotNet in general. |
The WebAuthComponent is now only included when the UNI_TASK symbol is defined, in addition to the existing Unity platform checks. This helps control compilation based on the presence of the UniTask library.
Rewrote the setup assistant and window logic to use reliable asynchronous package management with callbacks, improved UI responsiveness, and removed legacy code paths. The assistant now prevents concurrent operations, provides better user feedback, and ensures package installation and status checks are robust and non-blocking. The setup window UI is updated to reflect busy states and uses the new async methods for package installation.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Unity as a new SDK target and integrates Unity SDK generation into the example generator and CI. Introduces a new language provider class Appwrite\SDK\Language\Unity, many Unity templates (Editor UI/setup assistant, asmdef, csc, package manifest/lock, ProjectSettings, README/CHANGELOG/LICENSE, runtime Client, CookieContainer, Realtime, WebAuthComponent, services, manager, utilities, samples, WebGL plugins, docs examples), Unity test assets and a Unity2021 CI matrix entry, and a UNITY_TEST_MODE generation filter. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to pay attention to:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
♻️ Duplicate comments (1)
tests/languages/unity/Tests.cs (1)
56-75: Eliminate flakiness: replace fixed delays with a completion signal + timeoutWaiting a fixed 5s can be flaky and slow. Use a TaskCompletionSource signaled by the callback and race it with a timeout.
- string realtimeResponse = "No realtime message received within timeout"; - RealtimeSubscription subscription = null; - subscription = realtime.Subscribe(new [] { "tests" }, (eventData) => + var tcs = new TaskCompletionSource<string>(); + RealtimeSubscription subscription = null; + subscription = realtime.Subscribe(new [] { "tests" }, (eventData) => { Debug.Log($"[Test] Realtime callback invoked! Payload count: {eventData.Payload?.Count}"); if (eventData.Payload != null && eventData.Payload.TryGetValue("response", out var value)) { Debug.Log($"[Test] Found response value: {value}"); - realtimeResponse = value.ToString(); - Debug.Log($"[Test] Updated realtimeResponse to: {realtimeResponse}"); + tcs.TrySetResult(value.ToString()); } else { Debug.Log("[Test] No 'response' key found in payload"); } subscription?.Close(); }); - - await Task.Delay(5000); + var completed = await Task.WhenAny(tcs.Task, Task.Delay(TimeSpan.FromSeconds(10))); + var realtimeResponse = completed == tcs.Task ? tcs.Task.Result : "No realtime message received within timeout"; @@ - await Task.Delay(5000); - Debug.Log(realtimeResponse); + Debug.Log(realtimeResponse);Also applies to: 185-189
🧹 Nitpick comments (28)
templates/unity/LICENSE.twig (1)
1-1: License rendering is fine; consider avoiding escaping.If the generator enables auto-escaping for text templates, render raw to preserve formatting.
-{{sdk.license}} +{{ sdk.license|default('')|raw }}templates/unity/ProjectSettings/ProjectVersion.txt (1)
1-2: Parameterize Unity Editor version to reduce maintenance.Hardcoding the exact patch ties the generator to a single image/tag. Make this a Twig template and feed from config, with sensible defaults.
-m_EditorVersion: 2021.3.45f1 -m_EditorVersionWithRevision: 2021.3.45f1 (3409e2af086f) +m_EditorVersion: {{ unity.editorVersion|default('2021.3.45f1') }} +m_EditorVersionWithRevision: {{ unity.editorVersionWithRevision|default('2021.3.45f1 (3409e2af086f)') }}Also consider targeting a current LTS in CI while still allowing generation for older versions via config.
templates/unity/Assets/Runtime/Core/csc.rsp (1)
1-1: Confirm csc.rsp pickup scope and add trailing newline.Unity picks up csc.rsp files under Assets, but placement nuances exist across versions. Please verify this file is detected under Assets/Runtime/Core for 2021.3; if not, move a single csc.rsp to Assets/ to apply project-wide. Add a newline at EOF to avoid diff churn.
.github/workflows/tests.yml (1)
53-53: Matrix entry name implies version-lock; consider generalizing.If you’ll test multiple Unity LTS versions over time, consider “Unity” with a separate matrix dimension for version, or generate from a variable. This reduces churn across files.
templates/unity/Assets/Runtime/Core/Appwrite.Core.asmdef.twig (1)
4-6: UniTask reference vs optional dependency. Decide one path and align.Referencing “UniTask” here hard-requires the package; the versionDefines block implies it should be optional. Either:
- Keep the reference and declare the dependency in your Unity package (package.json) so consumers get UniTask automatically, or
- Make UniTask truly optional: remove the reference and guard all usages behind
#if UNI_TASKwith no public API surface exposing UniTask types when absent.If choosing optional, apply:
- "references": [ - "UniTask" - ], + "references": [],If choosing required, add UniTask to package.json “dependencies” (see comment in package.json.twig).
templates/unity/Assets/Editor/Appwrite.Editor.asmdef.twig (1)
2-7: Set rootNamespace to the Editor namespace for consistency.Editor assemblies typically use a distinct namespace to avoid collisions with runtime types.
- "name": "{{ spec.title | caseUcfirst }}.Editor", - "rootNamespace": "{{ spec.title | caseUcfirst }}", + "name": "{{ spec.title | caseUcfirst }}.Editor", + "rootNamespace": "{{ spec.title | caseUcfirst }}.Editor",templates/unity/package.json.twig (2)
2-7: Package identity should be vendor-agnostic and configurable.Hardcoding
com.fellmonkeyisn’t ideal for the generator. Expose a vendor/namespace input and default to org-owned value.- "name": "com.fellmonkey.{{spec.title | caseLower}}-sdk", + "name": "{{ sdk.packageName|default('com.appwrite.' ~ (spec.title | caseLower) ~ '-sdk') }}", "version": "{{sdk.version}}", "displayName": "{{spec.title}} SDK", "description": "{{spec.description}}", - "unity": "2021.3", + "unity": "{{ sdk.unityMinVersion|default('2021.3') }}",
8-17: Add runtime UPM dependencies to templates/unity/package.json.twigInsert a "dependencies" block immediately after the "keywords" array in templates/unity/package.json.twig declaring com.cysharp.unitask and com.endel.nativewebsocket (use the same UPM URLs/versions from templates/unity/Packages/manifest.json) so consumers auto-install required packages.
templates/unity/Packages/packages-lock.json (1)
1-490: Reconsider shipping a lockfile in a generator template.packages-lock.json tightly pins the dependency graph to Unity 2021.3 and specific package states. For a cross-version template, this can cause resolution failures or needless churn. Prefer:
- Omit this file from the template (let UPM resolve from manifest), or
- Generate it conditionally per targeted Unity editor in CI and don’t commit it.
templates/unity/Packages/manifest.json (2)
5-15: Trim dev-only/editor convenience packages from a library template.Consider removing non-essential deps to reduce install time and conflicts:
- com.unity.collab-proxy
- com.unity.ide.rider / com.unity.ide.visualstudio / com.unity.ide.vscode
- com.unity.feature.2d (and associated 2D extras) unless samples require them
- com.unity.visualscripting
This keeps the SDK lean; samples can declare extras via a Samples~ package or Setup Assistant.
If samples actually require these, keep them but scope to Samples~/sampleProject/manifest.json to avoid polluting consumer projects.
15-46: Scope core modules to the minimum needed.For an HTTP/WebSocket SDK, builtin modules likely needed are: jsonserialize, unitywebrequest, unitywebrequesttexture, (optionally) imgui/ugui for samples. Others (audio, physics, terrain, etc.) can be omitted.
templates/unity/README.md.twig (2)
6-6: Swap Travis badge for GitHub Actions.Project CI is on GitHub Actions per PR notes; the Travis badge is misleading.
-[](https://travis-ci.com/appwrite/sdk-generator) +[](https://github.com/appwrite/sdk-generator/actions/workflows/tests.yml)
5-5: Make the Unity version badge configurable.Hardcoding 2021.3+ is at odds with reviewer feedback to target newer LTS.
- + }}-blue.svg)Set sdk.unityMinVersion in the language provider to align with the chosen LTS (2022.3+ or Unity 6).
tests/languages/unity/Tests.cs (3)
117-121: Avoid unsafe cast for Redirect() resultGuard the cast to prevent NullReferenceException in case the return type changes.
- var result = await general.Redirect(); - Debug.Log((result as Dictionary<string, object>)["result"]); + var result = await general.Redirect(); + if (result is Dictionary<string, object> dict && dict.TryGetValue("result", out var res)) + Debug.Log(res); + else + Assert.Fail("Unexpected redirect payload.");
255-258: Prefer Destroy over DestroyImmediate in PlayModeIn PlayMode tests, use Destroy to avoid editor-only semantics.
- Object.DestroyImmediate(realtimeObject); + Object.Destroy(realtimeObject);
30-35: Surface original async exceptionThrowing task.Exception rethrows AggregateException. Prefer GetAwaiter().GetResult() to preserve original stack for single-fault tasks.
- if (task.Exception != null) - { - Debug.LogError($"Test failed with exception: {task.Exception}"); - throw task.Exception; - } + if (task.IsFaulted) + { + Debug.LogError($"Test failed with exception: {task.Exception}"); + task.GetAwaiter().GetResult(); // throws original + }tests/Unity2021Test.php (1)
20-20: Fail fast when UNITY_LICENSE is missing; preserve exit codes through pipesCurrently, tests proceed even without a valid license. Add an explicit check and pipefail to avoid false greens.
-protected string $command = 'docker run --network="mockapi" --rm -v "$(pwd):/project" -w /project/tests/sdks/unity -e UNITY_LICENSE unityci/editor:ubuntu-2021.3.45f1-base-3.1.0 /bin/bash -c "echo \"\$UNITY_LICENSE\" > Unity_lic.ulf && /opt/unity/Editor/Unity -nographics -batchmode -manualLicenseFile Unity_lic.ulf -quit || true && /opt/unity/Editor/Unity -projectPath . -batchmode -nographics -runTests -testPlatform PlayMode -stackTraceLogType None -logFile - 2>/dev/null | sed -n \'/Test Started/,\$p\' | grep -v -E \'^(UnityEngine\\.|System\\.|Cysharp\\.|\\(Filename:|\\[.*\\]|##utp:|^\\s*\$|The header Origin is managed automatically|Connected to realtime:)\' | grep -v \'StackTraceUtility\'"'; +protected string $command = 'docker run --network="mockapi" --rm -v "$(pwd):/project" -w /project/tests/sdks/unity -e UNITY_LICENSE unityci/editor:ubuntu-2021.3.45f1-base-3.1.0 /bin/bash -lc "\ +set -euo pipefail; \ +if [ -z \"${UNITY_LICENSE:-}\" ]; then echo \"UNITY_LICENSE secret missing\" >&2; exit 1; fi; \ +printf \"%s\" \"\$UNITY_LICENSE\" > Unity_lic.ulf; \ +/opt/unity/Editor/Unity -nographics -batchmode -manualLicenseFile Unity_lic.ulf -quit || true; \ +/opt/unity/Editor/Unity -projectPath . -batchmode -nographics -runTests -testPlatform PlayMode -stackTraceLogType None -logFile - \ + 2>/dev/null | sed -n \'/Test Started/,\$p\' | grep -v -E \'^(UnityEngine\\.|System\\.|Cysharp\\.|\\(Filename:|\\[.*\\]|##utp:|^\\s*\$|The header Origin is managed automatically|Connected to realtime:)\' | grep -v \'StackTraceUtility\'"';templates/unity/Assets/Runtime/Core/Plugins/WebGLCookies.jslib (1)
7-37: Consider more granular error handling in EnableWebGLHttpCredentialsWhile the function has a try-catch block, the empty catch could mask legitimate errors that developers should know about during debugging.
Consider logging errors in development builds:
- } catch (e) { /* noop */ } + } catch (e) { + if (typeof console !== 'undefined' && console.error) { + console.error('[Appwrite] Failed to enable WebGL HTTP credentials:', e); + } + }src/SDK/Language/Unity.php (1)
390-405: Test mode filtering logic could be more maintainableThe test mode filtering uses string matching on template destinations, which could break if template paths change. Consider using a more robust approach.
Consider defining test exclusions by scope or adding a test-specific property to file definitions:
// Alternative approach: Add 'excludeInTest' property to file definitions $files = [ [ 'scope' => 'default', 'destination' => 'Assets/Runtime/Utilities/{{ spec.title | caseUcfirst }}Utilities.cs', 'template' => 'unity/Assets/Runtime/Utilities/AppwriteUtilities.cs.twig', 'excludeInTest' => true, // Add this property ], // ... other files ]; // Then filter more simply: if (isset($GLOBALS['UNITY_TEST_MODE']) && $GLOBALS['UNITY_TEST_MODE'] === true) { $files = array_filter($files, function ($file): bool { return !($file['excludeInTest'] ?? false); }); }templates/unity/Assets/Samples~/AppwriteExample/AppwriteExample.cs.twig (1)
17-19: Fix formatting: Remove extra blank lineThere's an unnecessary blank line between the method signature and opening brace.
private async void Start() - {templates/unity/Assets/Runtime/Utilities/AppwriteUtilities.cs.twig (1)
35-36: Remove unused variable assignmentLine 36 assigns
manager.Realtimeto variableawhich is never used. This appears to be leftover debug/test code.- //Create Realtime instance - var a =manager.Realtime;templates/unity/Assets/Runtime/Realtime.cs.twig (1)
358-383: Async void pattern in UniTask.CreateUsing
UniTask.Createwith an async lambda creates a fire-and-forget task. Consider storing the task reference for proper lifecycle management.Consider storing the heartbeat task:
private UniTask _heartbeatTask; private void StartHeartbeat() { StopHeartbeat(); _heartbeatTokenSource = new CancellationTokenSource(); _heartbeatTask = UniTask.Create(async () => { // ... existing heartbeat logic ... }); }templates/unity/Assets/Editor/AppwriteSetupWindow.cs.twig (1)
250-251: Potential race condition in delayed message clearingUsing
Task.DelaywithTaskScheduler.FromCurrentSynchronizationContext()for clearing messages could fail if the synchronization context changes or becomes unavailable.Consider using Unity's coroutine system or EditorApplication callbacks:
private void ShowMessage(string message, MessageType type) { _statusMessage = message; _statusMessageType = type; Repaint(); if (type != MessageType.Error) { EditorApplication.delayCall += () => { EditorCoroutineUtility.StartCoroutine(ClearMessageAfterDelay(message, 5f), this); }; } } private System.Collections.IEnumerator ClearMessageAfterDelay(string message, float delay) { yield return new EditorWaitForSeconds(delay); if (_statusMessage == message) { _statusMessage = ""; Repaint(); } }templates/unity/Assets/Runtime/AppwriteManager.cs.twig (1)
136-137: Hardcoded assumption about service namespaceThe code assumes all services are in the same namespace as the
Accountclass. This assumption should be documented or made more flexible.Consider adding a comment to document this assumption:
+ // IMPORTANT: This assumes all service classes are in the same namespace as Account. + // If services are moved to different namespaces, this logic will need to be updated. var serviceNamespace = typeof(Account).Namespace; // Assumes all services are in the same namespace.templates/unity/Assets/Runtime/AppwriteConfig.cs.twig (1)
38-41: Consider HTTPS default for cloud endpoints.Using HTTPS by default aligns with security best practices. The realtime endpoint correctly uses
wss://for secure WebSocket connections.- [SerializeField] private string endpoint = "https://cloud.{{ spec.title | lower }}.io/v1"; + [SerializeField] private string endpoint = "https://cloud.{{ spec.title | lower }}.io/v1"; [Tooltip("WebSocket endpoint for realtime updates (optional)")] [SerializeField] private string realtimeEndpoint = "wss://cloud.{{ spec.title | lower }}.io/v1";templates/unity/Assets/Editor/AppwriteSetupAssistant.cs.twig (2)
144-152: Consider adding timeout handling for package installation.While the current implementation is functional, long-running package installations could benefit from timeout handling to prevent indefinite waiting.
Consider adding a timeout mechanism:
private static void InstallPackage(string packageUrl, Action onCompleted, float timeout = 60f) { if (_isBusy) return; var queue = new Queue<string>(); queue.Enqueue(packageUrl); var startTime = EditorApplication.timeSinceStartup; InstallNextPackage(queue, () => onCompleted?.Invoke(), (error) => { if (EditorApplication.timeSinceStartup - startTime > timeout) { Debug.LogError($"Package installation timed out after {timeout} seconds"); } Debug.LogError(error); }); }
13-16: Verified — package URLs are valid and actively maintained.
Both UniTask and NativeWebSocket UPM Git URLs are documented in their upstream READMEs and show recent activity; no immediate change required. Optional: document a fallback (pinned version or alternate registry) for resilience.templates/unity/Assets/Runtime/Core/Client.cs.twig (1)
725-731: Consider limiting certificate acceptance scope.While accepting all certificates works for self-signed scenarios, consider validating against a specific certificate fingerprint or CA for better security.
public class AcceptAllCertificatesSignedWithASpecificKeyPublicKey : CertificateHandler { + // Consider adding constructor to accept specific certificate fingerprint + private readonly byte[] _allowedFingerprint; + + public AcceptAllCertificatesSignedWithASpecificKeyPublicKey(byte[] allowedFingerprint = null) + { + _allowedFingerprint = allowedFingerprint; + } + protected override bool ValidateCertificate(byte[] certificateData) { - return true; // Accept all certificates + if (_allowedFingerprint == null) return true; // Accept all if no fingerprint specified + + // Validate against specific fingerprint for better security + // Implementation would compute hash of certificateData and compare + return true; // Simplified for now } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
templates/unity/Assets/Runtime/Core/Plugins/Microsoft.Bcl.AsyncInterfaces.dllis excluded by!**/*.dlltemplates/unity/Assets/Runtime/Core/Plugins/System.IO.Pipelines.dllis excluded by!**/*.dlltemplates/unity/Assets/Runtime/Core/Plugins/System.Runtime.CompilerServices.Unsafe.dllis excluded by!**/*.dlltemplates/unity/Assets/Runtime/Core/Plugins/System.Text.Encodings.Web.dllis excluded by!**/*.dlltemplates/unity/Assets/Runtime/Core/Plugins/System.Text.Json.dllis excluded by!**/*.dlltemplates/unity/ProjectSettings/AudioManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/ClusterInputManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/DynamicsManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/EditorBuildSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/EditorSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/GraphicsSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/InputManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/MemorySettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/NavMeshAreas.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/NetworkManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/PackageManagerSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/Physics2DSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/PresetManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/ProjectSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/QualitySettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/TagManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/TimeManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/UnityConnectSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/VFXManager.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/VersionControlSettings.assetis excluded by!**/*.assettemplates/unity/ProjectSettings/XRSettings.assetis excluded by!**/*.assettemplates/unity/icon.pngis excluded by!**/*.png
📒 Files selected for processing (32)
.github/workflows/tests.yml(2 hunks)example.php(2 hunks)src/SDK/Language/Unity.php(1 hunks)templates/unity/Assets/Editor/Appwrite.Editor.asmdef.twig(1 hunks)templates/unity/Assets/Editor/AppwriteSetupAssistant.cs.twig(1 hunks)templates/unity/Assets/Editor/AppwriteSetupWindow.cs.twig(1 hunks)templates/unity/Assets/Runtime/Appwrite.asmdef.twig(1 hunks)templates/unity/Assets/Runtime/AppwriteConfig.cs.twig(1 hunks)templates/unity/Assets/Runtime/AppwriteManager.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/Appwrite.Core.asmdef.twig(1 hunks)templates/unity/Assets/Runtime/Core/Client.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/Plugins/Android/AndroidManifest.xml(1 hunks)templates/unity/Assets/Runtime/Core/Plugins/WebGLCookies.jslib(1 hunks)templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/WebAuthComponent.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/csc.rsp(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)templates/unity/Assets/Runtime/Utilities/AppwriteUtilities.cs.twig(1 hunks)templates/unity/Assets/Samples~/AppwriteExample/AppwriteExample.cs.twig(1 hunks)templates/unity/CHANGELOG.md.twig(1 hunks)templates/unity/LICENSE.twig(1 hunks)templates/unity/Packages/manifest.json(1 hunks)templates/unity/Packages/packages-lock.json(1 hunks)templates/unity/ProjectSettings/ProjectVersion.txt(1 hunks)templates/unity/README.md.twig(1 hunks)templates/unity/base/requests/oauth.twig(1 hunks)templates/unity/docs/example.md.twig(1 hunks)templates/unity/package.json.twig(1 hunks)tests/Unity2021Test.php(1 hunks)tests/languages/unity/Tests.asmdef(1 hunks)tests/languages/unity/Tests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unity2021Test.php (2)
tests/languages/unity/Tests.cs (1)
Tests(16-261)tests/Base.php (1)
Base(17-275)
src/SDK/Language/Unity.php (3)
src/SDK/SDK.php (1)
SDK(21-933)src/SDK/Language.php (1)
Language(5-140)src/SDK/Language/DotNet.php (1)
DotNet(8-457)
example.php (2)
src/SDK/SDK.php (16)
SDK(21-933)setName(254-259)setDescription(265-270)setShortDescription(276-281)setURL(386-391)setLogo(375-380)setLicenseContent(320-325)setWarning(441-446)setChangelog(474-479)setVersion(287-292)setGitUserName(353-358)setGitRepoName(342-347)setTwitter(509-514)setDiscord(497-503)setDefaultHeaders(232-237)generate(615-732)src/SDK/Language/Unity.php (1)
Unity(5-409)
🔇 Additional comments (25)
templates/unity/package.json.twig (1)
18-24: Samples entry looks good.Path and metadata are aligned with Unity packaging conventions.
templates/unity/README.md.twig (1)
110-112: No change required — generated client exposes SetEndPointRealtime.
Found client.SetEndPointRealtime("wss://cloud.appwrite.io/v1") in tests/languages/unity/Tests.cs:49.templates/unity/CHANGELOG.md.twig (1)
1-1: LGTM.Simple passthrough of sdk.changelog. No issues.
example.php (2)
25-25: Unity language import — LGTMImport is correct and consistent with other languages.
80-104: Unity example generation block — LGTMParameters mirror other examples and are sane for local runs.
tests/languages/unity/Tests.cs (1)
49-50: Verify realtime endpoint setter nameMethod is
SetEndPointRealtimehere but the client usesSetEndpoint(...)elsewhere. Confirm the exact API to avoid a compile error (typo in “EndPoint” vs “Endpoint”?).tests/Unity2021Test.php (1)
22-28: UNITY_TEST_MODE toggle — LGTMThis keeps generation minimal for CI and aligns with Unity language provider behavior.
templates/unity/base/requests/oauth.twig (1)
7-7: Confirm Authenticate() return typeCode expects a Uri (uses callbackUri.Query). Verify WebAuthComponent.Authenticate(authUrl) returns Uri, or wrap with new Uri(string).
tests/languages/unity/Tests.asmdef (1)
1-23: LGTM! Assembly definition is properly configuredThe assembly definition correctly references all necessary dependencies including test runners, UniTask, and the main Appwrite assemblies. The configuration with
overrideReferences: trueand explicit NUnit reference is appropriate for Unity test assemblies.src/SDK/Language/Unity.php (3)
5-13: LGTM! Proper inheritance from DotNetThe Unity language class correctly extends DotNet, which makes sense given Unity's C# foundation. This inheritance allows code reuse while maintaining Unity-specific customizations.
20-33: Unity-specific keywords are appropriateThe keywords list correctly includes Unity-specific reserved words that should be escaped in generated code. The deduplication with
array_uniqueensures no duplicates from the parent class.
116-117: Good practice: Reusing DotNet templates where applicableThe file correctly reuses many DotNet templates for shared functionality (Exception, ID, Permission, Query, Role, converters, etc.), which promotes code reuse and maintainability as suggested in past reviews.
Also applies to: 121-122, 125-127, 130-132, 135-137, 145-147, 149-152, 155-157, 160-162, 165-167, 170-172, 175-177, 185-187, 190-192, 200-202
templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig (1)
70-76: Platform-specific stubs are well implementedGood use of conditional compilation to provide appropriate fallbacks for unsupported platforms, with clear warning messages for developers.
templates/unity/Assets/Samples~/AppwriteExample/AppwriteExample.cs.twig (1)
60-61: Good practice: Service examples are commented outThe commented-out service creation examples provide helpful guidance without generating non-compilable code. This is a good approach for template-based examples.
Also applies to: 106-107
templates/unity/Assets/Runtime/AppwriteConfig.cs.twig (3)
55-56: Security warning is appropriate for dev keys.Good security practice to warn about storing sensitive keys in ScriptableObjects. The warning message properly alerts developers to the security risk.
103-129: Well-structured editor utility for configuration creation.The CreateConfiguration method properly handles directory creation, unique asset naming, and provides good user feedback. The use of the Resources folder ensures runtime loading compatibility.
11-14: Fix incorrect bit masking for theMainandOthersservice flags.The current bit mask calculations are incorrect.
Mainshould cover bits 0-3 (Account, Databases, Functions, Storage), andOthersshould cover bits 4-8 (Avatars, Graphql, Locale, Messaging, Teams).Apply this fix:
- Main = (1 << 4) - 1, // 0-3 + Main = Account | Databases | Functions | Storage, // bits 0-3 [Tooltip("Selects all other services: Avatars, GraphQL, Locale, Messaging, Teams")] - Others = (1 << 9) - 1 ^ (1 << 4) - 1, // 4-8 + Others = Avatars | Graphql | Locale | Messaging | Teams, // bits 4-8Likely an incorrect or invalid review comment.
templates/unity/Assets/Editor/AppwriteSetupAssistant.cs.twig (2)
30-48: Robust initialization flow with proper state management.The initialization check properly handles editor states (compiling, updating) and uses EditorPrefs for persistent state management. The conditional display of the setup window based on package availability is well-implemented.
86-104: Excellent queue-based package installation implementation.The InstallAllPackages method properly handles concurrent operations, uses AssetDatabase.StartAssetEditing/StopAssetEditing for performance, and implements a clean recursive installation pattern with proper error handling.
templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig (3)
30-33: Good implementation of RFC 6265 cookie expiration logic.The IsExpired property correctly prioritizes Max-Age over Expires as per RFC 6265 specification.
73-86: Good platform-specific initialization.The WebGL-specific credential enabling and conditional cookie loading based on platform is well-implemented.
145-149: Cookie value parsing handles edge cases correctly.Using Split with count parameter of 2 properly handles cookie values containing '=' characters.
templates/unity/Assets/Runtime/Core/Client.cs.twig (3)
100-102: Good URL validation for endpoint setting.The validation ensures endpoints start with proper protocol schemes.
259-365: Comprehensive request preparation with proper multipart handling.The PrepareRequest method properly handles GET query strings, multipart form data with file uploads, JSON bodies, headers, cookies, and self-signed certificates. The implementation correctly processes InputFile from various sources (path, stream, bytes).
540-719: Excellent chunked upload implementation.The ChunkedUpload method properly:
- Handles resume functionality by checking existing upload progress
- Supports multiple input sources (path, stream, bytes)
- Implements proper Content-Range headers
- Provides progress callbacks
- Handles both small files (single upload) and large files (chunked)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
example.php (1)
98-101: Sync response-format header with selected spec version.
Hardcoding '1.6.0' risks drift from $version ('1.8.x' above). Derive '1.8.0' from $version to keep this example accurate.Apply this diff inside the Unity block:
- ->setDefaultHeaders([ - 'X-Appwrite-Response-Format' => '1.6.0', - ]) + ->setDefaultHeaders([ + 'X-Appwrite-Response-Format' => $responseFormat, + ])And add this once after the $version assignment (outside this hunk):
// Derive concrete response format (e.g., '1.8.x' -> '1.8.0') $responseFormat = preg_replace('/\.x$/', '.0', $version);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
example.php(2 hunks)
🔇 Additional comments (1)
example.php (1)
25-25: Unity provider import — LGTM.
Replaces 'async void' methods with non-async methods that call async methods using '.Forget()' to improve error handling and code clarity.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
templates/unity/base/requests/oauth.twig (2)
9-12: Replace System.Web.HttpUtility; not available on Unity’s .NET Standard
HttpUtility.ParseQueryStringwill not compile on Unity 2021/2022/6. Use a small parser instead.Apply:
- var query = HttpUtility.ParseQueryString(callbackUri.Query); - var secret = query.Get("secret"); - var key = query.Get("key"); - var callbackDomain = query.Get("domain"); // Get domain from callback + var pairs = callbackUri.Query.TrimStart('?').Split('&', StringSplitOptions.RemoveEmptyEntries); + var dict = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + foreach (var p in pairs) + { + var kv = p.Split('=', 2); + var k = Uri.UnescapeDataString(kv[0]); + var v = kv.Length > 1 ? Uri.UnescapeDataString(kv[1]) : string.Empty; + dict[k] = v; + } + dict.TryGetValue("secret", out var secret); + dict.TryGetValue("key", out var key); + dict.TryGetValue("domain", out var callbackDomain); // optional overrideNote: ensure
using System; using System.Collections.Generic;are present.
25-31: Gate cookie write in Editor to match the warningCurrently cookies are parsed even in Editor, contradicting the warning. Skip cookie creation in Editor.
Apply:
- var setCookieHeader = $"{key}={secret}; Path=/; Domain={parsedDomain}; Secure; HttpOnly; Max-Age={30 * 24 * 60 * 60}"; - Debug.Log($"Setting session cookie for domain: {parsedDomain}"); - _client.CookieContainer.ParseSetCookieHeader(setCookieHeader, parsedDomain); - -#if UNITY_EDITOR - Debug.LogWarning("[Appwrite] OAuth authorization in Editor: you can open and authorize, but cookies cannot be obtained. The session will not be set."); -#endif + #if UNITY_EDITOR + Debug.LogWarning("[Appwrite] OAuth in Editor: cookies are not persisted; session will not be set."); + #else + var setCookieHeader = $"{key}={secret}; Path=/; Domain={parsedDomain}; Secure; HttpOnly; Max-Age={30 * 24 * 60 * 60}"; + Debug.Log($"Setting session cookie for domain: {parsedDomain}"); + _client.CookieContainer.ParseSetCookieHeader(setCookieHeader, parsedDomain); + #endiftemplates/unity/Assets/Runtime/Realtime.cs.twig (3)
292-307: Null-guard_client/Configin fallback authAvoid potential NREs if
Initializehasn’t run orConfigis null.Apply:
private void SendFallbackAuthentication() { - var session = _client.Config.GetValueOrDefault("session"); + if (_client?.Config == null) + { + Debug.LogError("[Realtime] Cannot send fallback authentication: client/config is null"); + return; + } + var session = _client.Config.GetValueOrDefault("session");
463-466: Avoid async cleanup in OnDestroy; perform best-effort synchronous teardownUnity doesn’t await async work on destroy. Stop heartbeat/reconnect and close the socket immediately.
Apply:
- private void OnDestroy() - { - Disconnect().Forget(); - } + private void OnDestroy() + { + _reconnect = false; + StopHeartbeat(); + _cancellationTokenSource?.Cancel(); + if (_webSocket != null && _webSocket.State != WebSocketState.Closed) + { + // fire-and-forget close; Unity won't await here + _webSocket.Close(); + } + }
126-152: Channels added during in-flight connect can be missed; also reset reconnect flagIf
Subscribeis called while a connection is being established, the secondCreateSocket()returns early due to_creatingSocket, and the new channels are not included after the first connect completes. Also, after a previousCloseConnection(),_reconnectstays false; set it before connecting.Apply:
var subscription = new RealtimeSubscription { Channels = channels, OnMessage = callback, OnClose = () => CloseSubscription(subscriptionId, channels) }; @@ - CreateSocket().Forget(); + _reconnect = true; + if (_creatingSocket) + { + UniTask.Create(async () => + { + await UniTask.WaitUntil(() => !_creatingSocket); + await CreateSocket(); + }); + } + else + { + UniTask.Create(async () => { await CreateSocket(); }); + }
🧹 Nitpick comments (1)
templates/unity/Assets/Runtime/Realtime.cs.twig (1)
100-101: Unused cancellation token source
_cancellationTokenSourceis declared and cancelled but never created or used to drive operations. Either wire it into connect/heartbeat flows or remove it.Also applies to: 448-456
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
templates/unity/Assets/Runtime/AppwriteManager.cs.twig(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)templates/unity/Assets/Runtime/Utilities/AppwriteUtilities.cs.twig(1 hunks)templates/unity/Assets/Samples~/AppwriteExample/AppwriteExample.cs.twig(1 hunks)templates/unity/base/requests/oauth.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/unity/Assets/Samples~/AppwriteExample/AppwriteExample.cs.twig
- templates/unity/Assets/Runtime/Utilities/AppwriteUtilities.cs.twig
- templates/unity/Assets/Runtime/AppwriteManager.cs.twig
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
.github/workflows/tests.yml (1)
9-11: Guard Unity test execution when UNITY_LICENSE is unavailable.Forked PRs don't receive repository secrets, so Unity tests will fail when
UNITY_LICENSEis empty. This causes false negatives in CI.Apply a conditional guard to the "Run Tests" step:
- name: Run Tests + if: ${{ matrix.sdk != 'Unity2021' || env.UNITY_LICENSE != '' }} run: | composer test tests/${{ matrix.sdk }}Test.phptemplates/unity/README.md.twig (3)
39-39: Fix broken relative releases link.The relative link
/releasesbreaks when the README is viewed outside the repository (e.g., in the Unity Package Manager or on external sites).Apply this diff:
-1. Download the latest release from [GitHub](/releases) or zip +1. Download the latest release from [GitHub](https://github.com/{{ sdk.gitUserName|url_encode }}/{{ sdk.gitRepoName|url_encode }}/releases) or zip
120-126: Instantiate Realtime before use.The code references an undefined
realtimevariable.Apply this diff:
// Realtime example // You need to create a Realtime instance manually or attach dependently - realtime.Initialize(client); + var realtime = new Realtime(); + realtime.Initialize(client); var subscription = realtime.Subscribe(
133-133: Fix syntax error in code sample.The double dot
client..Async()is invalid C# syntax.Apply this diff:
- var result = await client..Async(); + var result = await client.Ping();templates/unity/Assets/Runtime/Realtime.cs.twig (5)
121-123: WebGL message dispatch guard may be inverted.The preprocessor guard prevents
DispatchMessageQueuefrom running on WebGL runtime builds. A past review indicated this is incorrect and events won't dispatch properly on WebGL.Apply this diff if the guard is indeed inverted:
- #if !UNITY_WEBGL || UNITY_EDITOR + #if UNITY_WEBGL && !UNITY_EDITOR _webSocket?.DispatchMessageQueue(); #endif
146-146: Race condition: concurrent Subscribe calls can trigger multiple socket creations.Calling
CreateSocket().Forget()without awaiting allows concurrentSubscribecalls to bypass the_creatingSocketflag check, which occurs after the async boundary.Consider making
Subscribeasync and awaiting the socket creation:- public RealtimeSubscription Subscribe(string[] channels, Action<RealtimeResponseEvent<Dictionary<string, object>>> callback) + public async UniTask<RealtimeSubscription> Subscribe(string[] channels, Action<RealtimeResponseEvent<Dictionary<string, object>>> callback) { // ... existing code ... - CreateSocket().Forget(); + await CreateSocket(); return subscription; }Update all call sites to await the new async signature.
289-304: Add null guard for _client before accessing Config.
SendFallbackAuthenticationdereferences_client.Configwithout checking if_clientis null, risking aNullReferenceException.Apply this diff:
private void SendFallbackAuthentication() { + if (_client == null) + { + Debug.LogError("[Realtime] Cannot send fallback authentication: client is null"); + return; + } + var session = _client.Config.GetValueOrDefault("session");
311-329: Marshal user callbacks to Unity main thread.On non-WebGL platforms,
NativeWebSocketinvokesOnMessageon a background thread. Invoking user callbacks directly from this thread can break Unity APIs (which require main thread) and cause data races.Wrap callback invocations to post to the main thread:
var subscriptionsCopy = _subscriptions.Values.ToArray(); + UniTask.Post(() => + { foreach (var subscription in subscriptionsCopy) { if (subscription.Channels.Any(subChannel => eventData.Channels.Contains(subChannel))) { subscription.OnMessage?.Invoke(eventData); } } + });
460-462: Avoid async void in OnDestroy—Unity won't wait for completion.Using async operations in
OnDestroyis problematic because Unity doesn't wait for them to complete before destroying the object, leading to incomplete cleanup.Replace with synchronous teardown:
- private void OnDestroy() + private void OnDestroy() { - Disconnect().Forget(); + _reconnect = false; + StopHeartbeat(); + _cancellationTokenSource?.Cancel(); + + if (_webSocket != null && _webSocket.State != WebSocketState.Closed) + { + Debug.LogWarning("[Realtime] WebSocket connection not properly closed before destruction"); + _webSocket.Close(); + } }templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig (2)
35-41: Refine domain matching to handle edge cases securely.The domain matching logic should validate domain structure and ensure subdomain matching is secure, particularly for domains starting with a dot.
Apply this diff:
public bool MatchesDomain(string requestDomain) { if (string.IsNullOrEmpty(Domain)) return true; var d = Domain.ToLowerInvariant(); var r = requestDomain.ToLowerInvariant(); - return r == d || r.EndsWith("." + d) || (d.StartsWith(".") && r.EndsWith(d)); + // Exact match or subdomain match + if (r == d) return true; + // For domain cookies (starting with .), check if request domain is a subdomain + if (d.StartsWith(".")) return r == d.Substring(1) || r.EndsWith(d); + // For non-dot domains, check if request is a subdomain + return r.EndsWith("." + d); }
135-136: Cookie parsing incorrectly splits on commas.Splitting Set-Cookie headers on commas breaks cookies with comma-containing values (e.g., date formats). HTTP Set-Cookie headers should be processed individually, not split.
Apply this diff:
public void ParseSetCookieHeader(string setCookieHeader, string domain) { if (string.IsNullOrWhiteSpace(setCookieHeader) || string.IsNullOrWhiteSpace(domain)) return; - foreach (var c in setCookieHeader.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)) - ParseCookie(c.Trim(), domain); + // Set-Cookie headers should be processed as a single cookie + // Multiple cookies come as multiple Set-Cookie headers, not comma-separated + ParseCookie(setCookieHeader.Trim(), domain); }Ensure callers iterate actual Set-Cookie header entries when multiple cookies are present.
🧹 Nitpick comments (1)
example.php (1)
121-144: Wrap Unity generation in conditional to match other SDK patterns.Unlike other SDKs, the Unity generation block always executes regardless of the
$requestedSdkargument. This causes unnecessary generation when users request a specific SDK.Apply this diff to match the pattern used by other SDKs:
- - // Unity - $sdk = new SDK(new Unity(), new Swagger2($spec)); - - $sdk - ->setName('NAME') - ->setDescription('Repo description goes here') - ->setShortDescription('Repo short description goes here') - ->setURL('https://example.com') - ->setLogo('https://appwrite.io/v1/images/console.png') - ->setLicenseContent('test test test') - ->setWarning('**WORK IN PROGRESS - NOT READY FOR USAGE**') - ->setChangelog('**CHANGELOG**') - ->setVersion('0.0.1') - ->setGitUserName('repoowner') - ->setGitRepoName('reponame') - ->setTwitter('appwrite_io') - ->setDiscord('564160730845151244', 'https://appwrite.io/discord') - ->setDefaultHeaders([ - 'X-Appwrite-Response-Format' => '1.6.0', - ]) - ; - - $sdk->generate(__DIR__ . '/examples/unity'); + // Unity + if (!$requestedSdk || $requestedSdk === 'unity') { + $sdk = new SDK(new Unity(), new Swagger2($spec)); + configureSDK($sdk); + $sdk->generate(__DIR__ . '/examples/unity'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/tests.yml(2 hunks)example.php(2 hunks)templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)templates/unity/README.md.twig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
example.php (1)
src/SDK/Language/Unity.php (1)
Unity(5-409)
Replaces reflection-based service initialization in AppwriteManager with direct instantiation for better performance and AOT compatibility. Adds thread safety to CookieContainer with locking, improves cookie loading/saving, and clarifies platform-specific behavior. Enhances Realtime to support client/session updates and controlled reconnect logic. Minor fixes include exception type consistency and improved debug messages in OAuth and other templates. Adds Operator.cs to Unity SDK file list.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
templates/unity/Assets/Runtime/AppwriteManager.cs.twig (2)
72-78: Start() init pattern looks good (no async-void).
247-269: Make Shutdown async and await realtime disconnect before destroying GameObject.Current flow can destroy the Realtime GameObject while
Disconnect()is still in-flight.- public async UniTask<bool> Reinitialize({{ spec.title | caseUcfirst }}Config newConfig = null, bool needRealtime = false) + public async UniTask<bool> Reinitialize({{ spec.title | caseUcfirst }}Config newConfig = null, bool needRealtime = false) { config = newConfig ?? config; - Shutdown(); + await Shutdown(); return await Initialize(needRealtime); } - private void Shutdown() + private async UniTask Shutdown() { if (!ReferenceEquals(_realtime, null)) { - _realtime.Disconnect().Forget(); + try { await _realtime.Disconnect(); } + catch (Exception ex) { Debug.LogError($"Error disconnecting realtime: {ex}"); } if (_realtime.gameObject != null) Destroy(_realtime.gameObject); } _realtime = null; _client = null; _isInitialized = false; _services.Clear(); OnClientDestroyed?.Invoke(); Debug.Log("{{ spec.title | caseUcfirst }} client shutdown"); }templates/unity/Assets/Runtime/Realtime.cs.twig (4)
147-155: WebGL DispatchMessageQueue guard is inverted (messages won’t dispatch on WebGL builds).
DispatchMessageQueue()needs to run on WebGL runtime builds; current preprocessor condition skips it.- #if !UNITY_WEBGL || UNITY_EDITOR - _webSocket?.DispatchMessageQueue(); - #endif + #if UNITY_WEBGL && !UNITY_EDITOR + _webSocket?.DispatchMessageQueue(); + #endif
276-310: Marshal message handling/user callbacks to Unity main thread (NativeWebSocket may invoke on background thread).Right now
HandleRealtimeEvent()invokessubscription.OnMessageinline. If this runs off-main-thread, it’s unsafe for Unity APIs and also races_subscriptions.A minimal approach is to
UniTask.Postthe dispatch:private void HandleRealtimeEvent(RealtimeResponseEvent<Dictionary<string, object>> eventData) { try { var subscriptionsCopy = _subscriptions.Values.ToArray(); - foreach (var subscription in subscriptionsCopy) - { - if (subscription.Channels.Any(subChannel => eventData.Channels.Contains(subChannel))) - { - subscription.OnMessage?.Invoke(eventData); - } - } + UniTask.Post(() => + { + foreach (var subscription in subscriptionsCopy) + { + if (subscription.Channels.Any(subChannel => eventData.Channels.Contains(subChannel))) + { + subscription.OnMessage?.Invoke(eventData); + } + } + }); } catch (Exception ex) { Debug.LogError($"[Realtime] HandleRealtimeEvent error: {ex.Message}"); OnError?.Invoke(ex); } }Also applies to: 345-363
323-338: Add defensive null-guards + don’t drop SendText failures.
SendFallbackAuthentication()assumes_clientand_client.Configexist, and it calls_webSocket.SendText(json)without awaiting/handling errors (depending on NativeWebSocket signature).private void SendFallbackAuthentication() { - var session = _client.Config.GetValueOrDefault("session"); + if (_client?.Config == null || _webSocket == null) + { + Debug.LogError("[Realtime] Cannot send fallback authentication: client/socket not ready"); + return; + } + + var session = _client.Config.GetValueOrDefault("session"); if (!string.IsNullOrEmpty(session)) { var authMessage = new RealtimeMessage<RealtimeAuthData> { Type = "authentication", Data = new RealtimeAuthData { Session = session } }; var json = JsonSerializer.Serialize(authMessage, Client.SerializerOptions); - _webSocket.SendText(json); + _ = _webSocket.SendText(json); // verify if this returns Task and decide whether to await/Forget } }NativeWebSocket (Unity) API: what are the return types/behavior of WebSocket.SendText(...) and WebSocket.Close(...), and on which thread does OnMessage fire for non-WebGL platforms?
510-513: Avoid fire-and-forget async cleanup in OnDestroy (Unity won’t wait).
Disconnect().Forget()can leave the socket open during teardown and can race with pending retries/heartbeat. Prefer a synchronous teardown (cancel tokens, stop heartbeat, best-effort close) or move async disconnect to an explicit shutdown path that callers can await.templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig (2)
144-149: Don’t split Set-Cookie on commas (breaks Expires and multi-cookie handling).Treat a single header value as one cookie; callers should pass individual Set-Cookie header entries (or split on newline, not comma, if Unity concatenates).
public void ParseSetCookieHeader(string setCookieHeader, string domain) { if (string.IsNullOrWhiteSpace(setCookieHeader) || string.IsNullOrWhiteSpace(domain)) return; - foreach (var c in setCookieHeader.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)) - ParseCookie(c.Trim(), domain); + ParseCookie(setCookieHeader.Trim(), domain); }
35-41: Harden domain matching (dot-domain semantics + subdomain safety).public bool MatchesDomain(string requestDomain) { if (string.IsNullOrEmpty(Domain)) return true; var d = Domain.ToLowerInvariant(); var r = requestDomain.ToLowerInvariant(); - return r == d || r.EndsWith("." + d) || (d.StartsWith(".") && r.EndsWith(d)); + if (r == d) return true; + if (d.StartsWith(".")) return r == d.Substring(1) || r.EndsWith(d); + return r.EndsWith("." + d); }templates/unity/Assets/Runtime/Core/Client.cs.twig (2)
460-469: Parse all Set-Cookie headers (Unity may return multiple / concatenated values).At minimum, use
GetResponseHeaders()and handle case-insensitive “Set-Cookie” keys; then pass each cookie string toCookieContainer.ParseSetCookieHeader()(after updating it to not comma-split).#if !(UNITY_WEBGL && !UNITY_EDITOR) - var setCookieHeader = request.GetResponseHeader("Set-Cookie"); - if (!string.IsNullOrEmpty(setCookieHeader)) - { - var uri = new Uri(request.url); - _cookieContainer.ParseSetCookieHeader(setCookieHeader, uri.Host); - } + var responseHeaders = request.GetResponseHeaders(); + if (responseHeaders != null) + { + var uri = new Uri(request.url); + foreach (var kvp in responseHeaders) + { + if (kvp.Key.Equals("Set-Cookie", StringComparison.OrdinalIgnoreCase) && + !string.IsNullOrWhiteSpace(kvp.Value)) + { + // If Unity concatenates, it's more likely newline-separated than comma-safe. + foreach (var line in kvp.Value.Split(new[] { '\n' }, StringSplitOptions.RemoveEmptyEntries)) + { + _cookieContainer.ParseSetCookieHeader(line.Trim(), uri.Host); + } + } + } + } #endif
646-709: ChunkedUpload corrupts the last chunk for stream/path sources (ignores bytesRead) + off-by-one for bytes source.For stream/path, you always send
buffer(full ChunkSize) even whenReadAsyncreturns fewer bytes; tail bytes are from previous chunk.while (offset < size) { + byte[] chunk; switch(input.SourceType) { case "path": case "stream": var stream = input.Data as Stream; if (stream == null) throw new InvalidOperationException("Stream data is null"); stream.Seek(offset, SeekOrigin.Begin); - await stream.ReadAsync(buffer, 0, ChunkSize); + var toRead = (int)Math.Min(ChunkSize, size - offset); + var bytesRead = await stream.ReadAsync(buffer, 0, toRead); + if (bytesRead <= 0) throw new EndOfStreamException("Unexpected end of stream during chunked upload"); + chunk = new byte[bytesRead]; + Buffer.BlockCopy(buffer, 0, chunk, 0, bytesRead); break; case "bytes": - buffer = ((byte[])input.Data) + chunk = ((byte[])input.Data) .Skip((int)offset) - .Take((int)Math.Min(size - offset, ChunkSize - 1)) + .Take((int)Math.Min(size - offset, ChunkSize)) .ToArray(); break; } var chunkHeaders = new Dictionary<string, string>(headers) { ["Content-Type"] = "multipart/form-data", - ["Content-Range"] = $"bytes {offset}-{Math.Min(offset + ChunkSize - 1, size - 1)}/{size}" + ["Content-Range"] = $"bytes {offset}-{Math.Min(offset + chunk.Length - 1, size - 1)}/{size}" }; ... - chunkParameters[paramName] = new InputFile + chunkParameters[paramName] = new InputFile { - Data = buffer, + Data = chunk, Filename = input.Filename, MimeType = input.MimeType, SourceType = "bytes" }; ... - offset += ChunkSize; + offset += chunk.Length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example.php(2 hunks)src/SDK/Language/Unity.php(1 hunks)templates/unity/Assets/Runtime/AppwriteManager.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/Client.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/CookieContainer.cs.twig(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)templates/unity/base/requests/oauth.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- example.php
- templates/unity/base/requests/oauth.twig
- src/SDK/Language/Unity.php
Refactored AppwriteManager to use a service definitions dictionary for cleaner and more maintainable service initialization. Renamed AcceptAllCertificatesSignedWithASpecificKeyPublicKey to AcceptAllCertificatesHandler for clarity. Enhanced Realtime reconnect logic to properly handle cancellation tokens, ensuring pending retry operations are cancelled and disposed when disconnecting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
templates/unity/Assets/Runtime/Realtime.cs.twig (4)
147-155: WebGL message pump preprocessor guard is inverted.The
#if !UNITY_WEBGL || UNITY_EDITORcondition causesDispatchMessageQueueto run on non-WebGL platforms and in Editor, but NativeWebSocket requires manual dispatching only on WebGL builds. On native platforms, messages are already dispatched on background threads.The guard should be inverted to dispatch on WebGL runtime only:
- #if !UNITY_WEBGL || UNITY_EDITOR - _webSocket?.DispatchMessageQueue(); - #endif + #if UNITY_WEBGL && !UNITY_EDITOR + _webSocket?.DispatchMessageQueue(); + #endif
323-338: Add defensive null check for_client.While
_clientshould be initialized viaInitialize()before any WebSocket operations, a defensive guard would prevent NullReferenceException if the component lifecycle is disrupted.private void SendFallbackAuthentication() { + if (_client == null) return; + var session = _client.Config.GetValueOrDefault("session");
345-363: User callbacks may execute on background thread.On non-WebGL platforms, NativeWebSocket's
OnMessagefires on a background thread. Invoking user callbacks directly can break Unity API calls and cause race conditions.Consider marshaling to the main thread:
private void HandleRealtimeEvent(RealtimeResponseEvent<Dictionary<string, object>> eventData) { try { var subscriptionsCopy = _subscriptions.Values.ToArray(); + // Post to main thread to ensure Unity API safety + UniTask.Post(() => + { foreach (var subscription in subscriptionsCopy) { if (subscription.Channels.Any(subChannel => eventData.Channels.Contains(subChannel))) { subscription.OnMessage?.Invoke(eventData); } } + }); }
540-543: Fire-and-forget in OnDestroy may cause incomplete cleanup.Unity doesn't wait for async operations in
OnDestroy. TheDisconnect().Forget()call may not complete before the object is destroyed, potentially leaving WebSocket connections open.Consider synchronous cleanup:
private void OnDestroy() { - Disconnect().Forget(); + // Perform synchronous cleanup since Unity won't wait for async + _reconnect = false; + StopHeartbeat(); + _cancellationTokenSource?.Cancel(); + _cancellationTokenSource?.Dispose(); + + if (_webSocket != null && _webSocket.State != WebSocketState.Closed) + { + _webSocket.Close(); + } }templates/unity/Assets/Runtime/AppwriteManager.cs.twig (1)
244-259: Fire-and-forget disconnect may leave connections open.The
Disconnect().Forget()call may not complete before the GameObject is destroyed. Given thatShutdownis called fromOnDestroy, async won't help here.If
Realtime.OnDestroyperforms synchronous cleanup (as suggested), this becomes less of a concern. Alternatively, expose a synchronousDisconnectImmediate()method on Realtime:- _realtime.Disconnect().Forget(); + _realtime.DisconnectImmediate(); // Synchronous cleanuptemplates/unity/Assets/Runtime/Core/Client.cs.twig (4)
261-262: Content-Type header lookup is case-sensitive.
headers.TryGetValue("Content-Type", ...)won't match"content-type"keys. While internal headers use lowercase, external callers might pass different casing.- var isMultipart = headers.TryGetValue("Content-Type", out var contentType) && - "multipart/form-data".Equals(contentType, StringComparison.OrdinalIgnoreCase); + var isMultipart = headers.Any(h => + h.Key.Equals("Content-Type", StringComparison.OrdinalIgnoreCase) && + h.Value != null && + h.Value.StartsWith("multipart/form-data", StringComparison.OrdinalIgnoreCase));
461-469: May miss multiple Set-Cookie headers.
GetResponseHeader("Set-Cookie")typically returns only the first header. Consider usingGetResponseHeaders()to capture all cookies:#if !(UNITY_WEBGL && !UNITY_EDITOR) - var setCookieHeader = request.GetResponseHeader("Set-Cookie"); - if (!string.IsNullOrEmpty(setCookieHeader)) - { - var uri = new Uri(request.url); - _cookieContainer.ParseSetCookieHeader(setCookieHeader, uri.Host); - } + var responseHeaders = request.GetResponseHeaders(); + if (responseHeaders != null && responseHeaders.TryGetValue("Set-Cookie", out var setCookieHeader)) + { + var uri = new Uri(request.url); + _cookieContainer.ParseSetCookieHeader(setCookieHeader, uri.Host); + } #endif
658-664: Off-by-one error in chunk size calculation.
ChunkSize - 1causes each chunk to be one byte smaller than intended, potentially causing data corruption over multiple chunks.case "bytes": buffer = ((byte[])input.Data) .Skip((int)offset) - .Take((int)Math.Min(size - offset, ChunkSize - 1)) + .Take((int)Math.Min(size - offset, ChunkSize)) .ToArray(); break;
721-728: Certificate handler name is misleading.
AcceptAllCertificatesHandleraccepts all certificates unconditionally, but the previous class name implied specific key validation. The current name is more accurate. However, this is still security-sensitive and should only be used for development.Consider adding a warning:
// Custom certificate handler for self-signed certificates + // WARNING: This bypasses certificate validation entirely. Use only for development/testing. public class AcceptAllCertificatesHandler : CertificateHandler { protected override bool ValidateCertificate(byte[] certificateData) { - return true; // Accept all certificates + Debug.LogWarning("[Security] Certificate validation bypassed - do not use in production!"); + return true; } }
🧹 Nitpick comments (2)
templates/unity/Assets/Runtime/Realtime.cs.twig (1)
157-182: Subscribe is synchronous but socket creation is fire-and-forget.While the
_creatingSocketflag prevents concurrent socket creation, callers cannot know when the connection is established. Consider returning aUniTask<RealtimeSubscription>or providing anIsConnectingstate so callers can await readiness.If synchronous return is intentional for simplicity, this is acceptable. Otherwise, consider:
public async UniTask<RealtimeSubscription> Subscribe(string[] channels, Action<RealtimeResponseEvent<Dictionary<string, object>>> callback) { // ... setup code ... await CreateSocket(); return subscription; }templates/unity/Assets/Runtime/AppwriteManager.cs.twig (1)
72-78: Consider logging initialization failures from Start.Using
Initialize().Forget()is the correct UniTask pattern, but failures will be silently swallowed. Consider wrapping in an error handler:private void Start() { if (initializeOnStart) { - Initialize().Forget(); + InitializeWithLogging().Forget(); } } + +private async UniTaskVoid InitializeWithLogging() +{ + var success = await Initialize(); + if (!success) + { + Debug.LogError("{{ spec.title | caseUcfirst }}Manager failed to initialize on Start"); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
templates/unity/Assets/Runtime/AppwriteManager.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/Client.cs.twig(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)
🔇 Additional comments (10)
templates/unity/Assets/Runtime/Realtime.cs.twig (3)
184-208: LGTM!The subscription cleanup logic correctly handles channel reference counting and appropriately closes the connection when no subscriptions remain.
210-258: LGTM!The socket creation logic properly guards against concurrent creation, handles URL changes, and includes appropriate error handling with retry.
423-461: LGTM!The retry logic now properly checks
_reconnectafter the delay and uses cancellation tokens to cancel pending retries when disconnecting, addressing the previous race condition concern.templates/unity/Assets/Runtime/AppwriteManager.cs.twig (3)
57-70: LGTM!The singleton pattern implementation correctly handles duplicate instances with appropriate warnings and cleanup.
80-126: LGTM!The initialization flow has proper validation, error handling, and success indication via return value.
128-156: LGTM!The factory-based service creation pattern avoids the IL2CPP reflection issues previously flagged with
Activator.CreateInstance.templates/unity/Assets/Runtime/Core/Client.cs.twig (4)
36-59: LGTM!The JSON serializer configuration with camelCase naming, null handling, and custom converters follows best practices.
61-87: LGTM!The constructor properly initializes all fields and loads persisted session data with appropriate error handling.
377-432: LGTM!The redirect method correctly disables auto-redirect and returns the Location header for OAuth flow handling.
443-535: LGTM!The
Call<T>method handles both JSON and binary responses appropriately with proper error extraction and optional type conversion.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Unity2021Test.php(1 hunks)tests/languages/unity/Tests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/languages/unity/Tests.cs
🔇 Additional comments (3)
tests/Unity2021Test.php (3)
1-13: LGTM! Configuration is consistent and well-structured.The Unity SDK metadata and language provider configuration are set up correctly.
14-18: Build steps look reasonable.The test asset preparation correctly copies Unity test files into the generated SDK structure.
30-45: LGTM! Comprehensive test coverage.The expected output array appropriately merges all response types from the base test class, ensuring comprehensive validation of Unity SDK functionality including HTTP operations, uploads/downloads, enums, exceptions, realtime, cookies, and helper utilities.
4 participants
What does this PR do?
This PR adds comprehensive Unity SDK support to the SDK generator by:
Unity Language Implementation: Introduces a new
Unitylanguage class (src/SDK/Language/Unity.php) that extends the baseLanguageclass, providing Unity-specific type mappings, keywords, and code generation logic for C# in Unity environment.Unity Template System: Adds a complete set of Unity-specific templates under
templates/unity/Assets/including:Automated Testing Integration: Introduces Unity2021 test support with:
Unity2021Test.phptest class that integrates with the existing test frameworkTests.cs,Tests.asmdef) for comprehensive SDK testingunityci/editor:ubuntu-2021.3.45f1-base-3.1.0CI/CD Integration: Updates the GitHub Actions workflow to:
UNITY_LICENSEenvironment variable for Unity Editor automationUnity Project Structure: Implements proper Unity package structure with:
Assets/Runtime/andAssets/Editor/Test Plan
The testing strategy includes:
Unit Testing: The
Unity2021Test.phpruns comprehensive tests covering:Docker-based CI Testing: Tests run in a controlled Unity environment using:
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.