[ECO-5624] Fix unity newtonsoft dependency#1313
Conversation
|
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 a Cake task that ilRepack-merges Unity-targeted DLLs into a single IO.Ably.dll and exposes Update.AblyUnity; replaces explicit updater copy scripts with that target; removes many Unity plugin .meta/.deps files; introduces UnityAdapter/UnityHelper for runtime detection; converts Agent runtime/os helpers to cached properties; migrates tests from FluentAssertions to NUnit and renames several example/test classes and asmdef references. Also updates build csproj for UNITY_PACKAGE, and adds/renames methods and a global variable in Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as Updater script (shell/cmd)
participant Cake as Cake
participant Build as NETStandard build output
participant Unity as Unity project (Assets/Ably/Plugins)
rect rgb(247,247,255)
Note over Updater,Unity: OLD flow (pre-change)
Updater->>Build: --target=Build.NetStandard --define=UNITY_PACKAGE
Updater->>Unity: copy IO.Ably.dll, IO.Ably.DeltaCodec.dll, *.pdb -> unity/Assets/Ably/Plugins
Updater->>Unity: update version file
end
rect rgb(240,255,240)
Note over Updater,Cake: NEW flow (post-change)
Updater->>Cake: --target=Update.AblyUnity
Cake->>Cake: run _Build_Ably_Unity_Dll
Cake->>Build: validate NETStandard2.0 bin & required DLLs
Cake->>Build: ilRepack merge assemblies -> produce merged IO.Ably.dll
Cake->>Unity: copy merged IO.Ably.dll -> unity/Assets/Ably/Plugins
Cake->>Unity: update version file
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
bf98850 to
86d5060
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
0808025 to
e6da13c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cake-build/tasks/build.cake (1)
107-154: Consider adding existence checks for all DLLs before merging.The task checks for the existence of the primary DLL and Newtonsoft.Json.dll but doesn't verify the existence of the other dependency DLLs (DeltaCodec, System.Runtime.CompilerServices.Unsafe, System.Threading.Channels, System.Threading.Tasks.Extensions) before attempting the merge. If any of these DLLs are missing, the merge operation may fail with a less descriptive error.
Consider adding explicit existence checks for all DLLs in the merge array:
var dllsToMerge = new[] { netStandard20BinPath.CombineWithFilePath("IO.Ably.DeltaCodec.dll"), netStandard20BinPath.CombineWithFilePath("System.Runtime.CompilerServices.Unsafe.dll"), netStandard20BinPath.CombineWithFilePath("System.Threading.Channels.dll"), netStandard20BinPath.CombineWithFilePath("System.Threading.Tasks.Extensions.dll"), newtonsoftDll }; + // Verify all DLLs exist before merging + foreach (var dll in dllsToMerge) + { + if (!FileExists(dll)) + { + throw new Exception($"Required DLL not found: {dll}. Please build the IO.Ably.NETStandard20 project first."); + } + } + var unityOutputPath = paths.Root.Combine("unity/Assets/Ably/Plugins");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (15)
lib/unity/AOT/Newtonsoft.Json.dllis excluded by!**/*.dlllib/unity/UnityEngine.dllis excluded by!**/*.dllunity/Assets/Ably/Examples/Dashboard/AblyDashboard.unityis excluded by!**/*.unityunity/Assets/Ably/Examples/Dashboard/logo.pngis excluded by!**/*.pngunity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/IO.Ably.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Threading.Channels.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dllis excluded by!**/*.dllunity/ProjectSettings/EditorBuildSettings.assetis excluded by!**/*.assetunity/ProjectSettings/PackageManagerSettings.assetis excluded by!**/*.assetunity/ProjectSettings/ProjectSettings.assetis excluded by!**/*.assetunity/doc_images/assembly_version_validation.PNGis excluded by!**/*.png
📒 Files selected for processing (32)
cake-build/tasks/build.cake(2 hunks)lib/unity/AOT/Newtonsoft.Json.dll.meta(1 hunks)src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj(1 hunks)unity-plugins-updater.cmd(1 hunks)unity-plugins-updater.sh(1 hunks)unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs(2 hunks)unity/Assets/Ably/Examples/Dashboard/AblyMain.cs(3 hunks)unity/Assets/Ably/Examples/Dashboard/AblyPresence.cs(2 hunks)unity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dll.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.xml.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dll.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.xml.meta(0 hunks)unity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dll.meta(0 hunks)unity/Assets/Ably/Plugins/System.Threading.Channels.dll.meta(0 hunks)unity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dll.meta(0 hunks)unity/Assets/Tests/AblySandbox/AblySandbox.asmdef(1 hunks)unity/Assets/Tests/EditMode/AblyInterfaceSpecs.cs(2 hunks)unity/Assets/Tests/EditMode/AuthSandboxSpecs.cs(30 hunks)unity/Assets/Tests/EditMode/EditMode.asmdef(1 hunks)unity/Assets/Tests/EditMode/StatsSandBoxSpecs.cs(1 hunks)unity/Assets/Tests/PlayMode/AblyInterfaceSpecs.cs(3 hunks)unity/Assets/Tests/PlayMode/AuthSandboxSpecs.cs(30 hunks)unity/Assets/Tests/PlayMode/PlayMode.asmdef(1 hunks)unity/Assets/Tests/PlayMode/StatsSandBoxSpecs.cs(1 hunks)unity/CONTRIBUTING.md(1 hunks)unity/Packages/manifest.json(1 hunks)unity/Packages/packages-lock.json(0 hunks)unity/README.md(2 hunks)
💤 Files with no reviewable changes (12)
- unity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dll.meta
- unity/Packages/packages-lock.json
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.xml.meta
- unity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dll.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.xml.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dll.meta
- unity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dll.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT.meta
- unity/Assets/Ably/Plugins/System.Threading.Channels.dll.meta
🧰 Additional context used
🧬 Code graph analysis (7)
unity/Assets/Tests/EditMode/AblyInterfaceSpecs.cs (1)
src/IO.Ably.Shared/Agent.cs (4)
Agent(11-297)OsIdentifier(159-258)UnityPlayerIdentifier(90-94)DotnetRuntimeIdentifier(46-87)
unity/Assets/Tests/PlayMode/StatsSandBoxSpecs.cs (1)
unity/Assets/Tests/AblySandbox/AblySandbox.cs (2)
AblySandbox(13-133)AblySandbox(17-29)
unity/Assets/Tests/PlayMode/AblyInterfaceSpecs.cs (1)
src/IO.Ably.Shared/Agent.cs (4)
Agent(11-297)OsIdentifier(159-258)UnityPlayerIdentifier(90-94)DotnetRuntimeIdentifier(46-87)
unity/Assets/Tests/EditMode/StatsSandBoxSpecs.cs (1)
unity/Assets/Tests/AblySandbox/AblySandbox.cs (2)
AblySandbox(13-133)AblySandbox(17-29)
unity/Assets/Tests/EditMode/AuthSandboxSpecs.cs (6)
src/IO.Ably.Shared/TokenDetails.cs (1)
IsValidToken(131-146)src/IO.Ably.Shared/Types/ErrorCodes.cs (1)
ErrorCodes(3-111)unity/Assets/Tests/AblySandbox/TaskCompletionAwaiter.cs (1)
SetCompleted(58-61)unity/Assets/Tests/PlayMode/AuthSandboxSpecs.cs (3)
Task(1014-1042)Task(1044-1067)Task(1069-1075)unity/Assets/Tests/AblySandbox/AblySandbox.cs (3)
Task(67-74)Task(76-88)Task(90-104)unity/Assets/Tests/AblySandbox/E7Assert.cs (2)
Task(9-12)Task(14-39)
unity/Assets/Tests/PlayMode/AuthSandboxSpecs.cs (5)
src/IO.Ably.Shared/TokenDetails.cs (1)
IsValidToken(131-146)src/IO.Ably.Shared/Types/ErrorCodes.cs (1)
ErrorCodes(3-111)unity/Assets/Tests/EditMode/AuthSandboxSpecs.cs (3)
Task(1013-1041)Task(1043-1066)Task(1068-1074)unity/Assets/Tests/AblySandbox/AblySandbox.cs (3)
Task(67-74)Task(76-88)Task(90-104)unity/Assets/Tests/AblySandbox/E7Assert.cs (2)
Task(9-12)Task(14-39)
unity/Assets/Ably/Examples/Dashboard/AblyMain.cs (2)
unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs (4)
AblyChannel(8-123)AblyChannel(22-26)AblyChannel(28-31)RegisterUiComponents(33-54)unity/Assets/Ably/Examples/Dashboard/AblyPresence.cs (4)
AblyPresence(7-142)AblyPresence(21-25)AblyPresence(27-30)RegisterUiComponents(32-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
🔇 Additional comments (27)
lib/unity/AOT/Newtonsoft.Json.dll.meta (1)
15-15: LGTM - Unity metadata format correction.The change from an implicit empty key to an explicit quoted empty string (
'') is a valid YAML format adjustment for Unity's PluginImporter metadata.unity/CONTRIBUTING.md (1)
151-154: LGTM - Documentation reflects new consolidated DLL workflow.The updated description accurately reflects the new build process where dependencies are merged into
IO.Ably.dllusing ILRepack, aligning with the PR objectives.unity/Assets/Tests/PlayMode/AuthSandboxSpecs.cs (2)
74-81: LGTM - FluentAssertions to NUnit migration.The assertion conversions are correctly implemented with proper argument order (expected, actual).
381-381: Correct use of Assert.AreSame for singleton comparison.Using
Assert.AreSameforErrorInfo.NonRenewableTokenis appropriate as it checks reference equality for what is likely a static/singleton error instance.unity/Assets/Tests/PlayMode/AblyInterfaceSpecs.cs (1)
221-243: LGTM - Correct FluentAssertions to NUnit migration.The assertion conversions maintain the correct argument order and test logic. The validation of agent header structure and format is preserved.
unity/Assets/Tests/PlayMode/PlayMode.asmdef (2)
10-12: Appropriate WebGL exclusion.Adding WebGL to
excludePlatformsis consistent with the documented platform limitations inunity/README.md, which notes that WebGL is not supported due to WebSocket incompatibility.
15-18: LGTM - Consolidated precompiled references.Reducing precompiled references to just
nunit.framework.dllandIO.Ably.dllaligns with the PR's goal of merging dependencies into a single Unity DLL.unity/Assets/Tests/AblySandbox/AblySandbox.asmdef (1)
9-12: LGTM - Consistent dependency consolidation.The reduction of precompiled references matches the pattern applied across all test assemblies, supporting the unified
IO.Ably.dllapproach.unity/README.md (2)
5-8: LGTM - Simplified system requirements.Removing the Unity Scripting Runtime Version requirement streamlines the documentation while retaining the essential API Compatibility Level requirement.
60-61: Updated demo reference.The change from chat demo to dashboard demo reflects the current example applications available in the repository.
unity/Assets/Tests/EditMode/EditMode.asmdef (1)
16-19: EditMode tests successfully migrated to NUnit.Verification confirms all EditMode test files (
AblyInterfaceSpecs.cs,AuthSandboxSpecs.cs,StatsSandBoxSpecs.cs) have been fully migrated to NUnit assertions. All assertion calls useAssert.*methods (AreEqual, IsTrue, IsFalse, IsNull) rather than FluentAssertions patterns. No FluentAssertions imports or BoundfoxStudios references remain. The removal ofBoundfoxStudios.FluentAssertionsfrom precompiled references is correct and justified.cake-build/tasks/build.cake (1)
182-185: LGTM!The public task provides a clear entry point for updating Unity DLLs with proper dependency on the internal build task.
unity/Assets/Ably/Examples/Dashboard/AblyPresence.cs (1)
7-30: LGTM!The class rename from
AblyPresenceUiConsoletoAblyPresenceimproves code clarity by removing redundant naming. The constructor and factory method have been updated consistently.unity/Assets/Tests/EditMode/AuthSandboxSpecs.cs (3)
73-80: LGTM!The FluentAssertions migration to NUnit assertions is correctly applied. The assertion logic remains functionally equivalent.
110-132: LGTM!The token validation assertions have been correctly migrated from FluentAssertions to NUnit. The test logic and expected behaviors are preserved.
161-169: LGTM!Error reason and connection state assertions have been properly converted to NUnit format with correct assertion methods.
unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs (1)
8-30: LGTM!The class rename from
AblyChannelUiConsoletoAblyChannelis consistent with the broader refactoring effort and improves naming clarity.unity/Assets/Ably/Examples/Dashboard/AblyMain.cs (1)
10-10: LGTM!The class rename from
AblyConsoletoAblyMainand the field type updates are consistent with the refactoring in the referenced classes (AblyChannelandAblyPresence). The factory method usage is correct.Also applies to: 22-34
unity/Packages/manifest.json (1)
1-60: LGTM!The removal of the FluentAssertions package and its scoped registry is consistent with the test migration from FluentAssertions to NUnit assertions throughout the Unity project.
unity-plugins-updater.cmd (1)
1-7: LGTM!The simplification to use the centralized
Update.AblyUnityCake target is a good improvement. This eliminates duplication and delegates the DLL merging and copying logic to the build system, making the script more maintainable.unity/Assets/Tests/PlayMode/StatsSandBoxSpecs.cs (1)
71-89: LGTM!The FluentAssertions to NUnit assertion migration is correctly applied with proper argument order (expected, actual) following NUnit conventions.
unity/Assets/Tests/EditMode/StatsSandBoxSpecs.cs (1)
70-87: LGTM! Clean migration to NUnit assertions.The FluentAssertions-to-NUnit migration maintains the correct test logic and aligns with the PR objective to enable Unity play mode testing on devices.
src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj (2)
64-64: LGTM! Path updated to align with Unity library reorganization.The HintPath change correctly reflects the new location of UnityEngine.dll under
lib/unity/.
68-72: Good addition to support ILRepack for Unity packaging.The conditional
CopyLocalLockFileAssembliessetting ensures NuGet dependencies are available in the output directory for ILRepack to merge. This correctly activates whenUNITY_PACKAGEis defined via the--define=UNITY_PACKAGEflag inunity-plugins-updater.sh(line 6).unity/Assets/Tests/EditMode/AblyInterfaceSpecs.cs (2)
224-224: LGTM! Correct assertion migration.The conversion from FluentAssertions to NUnit maintains the correct test logic for verifying the header values array length.
227-228: LGTM! Assertion migrations maintain test intent.The FluentAssertions-to-NUnit conversions are correct:
- String prefix checks properly migrated to
Assert.IsTrue(...StartsWith())- Count/length checks properly migrated to
Assert.AreEqual- Line 245 retains the descriptive failure message, which is good for debugging
Also applies to: 238-238, 242-242, 245-245
unity-plugins-updater.sh (1)
7-7: Good consolidation of the Unity plugin update workflow.Delegating to a Cake target is cleaner than explicit file copying. Ensure that the
Update.AblyUnitytarget correctly places all required artifacts (IO.Ably.dll, PDB files, etc.) intounity/Assets/Ably/Plugins.Run the following script to verify the Cake target exists and inspect its implementation:
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
5f03560 to
de00d64
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
1e7097b to
438dc8a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
438dc8a to
4df62c1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
61b484c to
7d0629b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj(1 hunks)src/IO.Ably.Shared/Agent.cs(5 hunks)src/IO.Ably.Shared/Unity/UnityAdapter.cs(1 hunks)src/IO.Ably.Shared/Unity/UnityHelper.cs(1 hunks)src/IO.Ably.Tests.Shared/AgentTests.cs(1 hunks)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs(2 hunks)src/IO.Ably.Tests.Shared/Rest/AblyHttpClientSpecs.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj
🧰 Additional context used
🧬 Code graph analysis (6)
src/IO.Ably.Tests.Shared/Rest/AblyHttpClientSpecs.cs (1)
src/IO.Ably.Shared/Agent.cs (1)
Agent(11-245)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs (1)
src/IO.Ably.Shared/Agent.cs (1)
Agent(11-245)
src/IO.Ably.Shared/Unity/UnityAdapter.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
RuntimePlatform(36-79)
src/IO.Ably.Shared/Agent.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
GetOsIdentifier(98-150)UnityHelper(9-166)
src/IO.Ably.Tests.Shared/AgentTests.cs (1)
src/IO.Ably.Shared/Agent.cs (1)
Agent(11-245)
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
src/IO.Ably.Shared/Agent.cs (1)
GetOsIdentifier(107-206)src/IO.Ably.Shared/Unity/UnityAdapter.cs (1)
UnityAdapter(26-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
🔇 Additional comments (1)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs (1)
133-134: LGTM! Property access correctly replaces method calls.The conversion from method calls to property access aligns with the Agent.cs refactoring that introduces cached, lazy-loaded properties for runtime and OS identifiers.
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
1c0aa6c to
9459200
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1313/features
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
unity/Assets/Ably/Plugins/IO.Ably.dllis excluded by!**/*.dll
📒 Files selected for processing (7)
src/IO.Ably.Shared/IO.Ably.Shared.projitems(1 hunks)src/IO.Ably.Shared/Unity/UnityAdapter.cs(1 hunks)src/IO.Ably.Shared/Unity/UnityHelper.cs(1 hunks)unity/Assets/Ably/Plugins/link.xml(1 hunks)unity/Assets/Tests/AblySandbox/AblyRealtimeExtensions.cs(0 hunks)unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs(2 hunks)unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs(3 hunks)
💤 Files with no reviewable changes (1)
- unity/Assets/Tests/AblySandbox/AblyRealtimeExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs
🧰 Additional context used
🧬 Code graph analysis (3)
unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs (2)
src/IO.Ably.Shared/Agent.cs (1)
Agent(11-245)src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
UnityHelper(9-166)
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
src/IO.Ably.Shared/Agent.cs (1)
GetOsIdentifier(107-206)src/IO.Ably.Shared/Unity/UnityAdapter.cs (1)
UnityAdapter(26-132)
src/IO.Ably.Shared/Unity/UnityAdapter.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
RuntimePlatform(36-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
🔇 Additional comments (8)
unity/Assets/Ably/Plugins/link.xml (1)
3-8: LGTM! Linker configuration correctly preserves Unity reflection targets.The addition of
UnityEngine.CoreModulewithUnityEngine.Applicationtype and itsunityVersionandplatformproperties is correctly aligned with the reflection-based access inUnityAdapter.cs. This ensures IL2CPP builds won't strip these properties that are accessed dynamically.src/IO.Ably.Shared/Unity/UnityAdapter.cs (3)
58-72: Verify Unity version support and consider adding fallbacks.The comment on line 63 states Unity 2017.1, but according to Unity's upgrade documentation, the assembly split to
UnityEngine.CoreModuleoccurred in Unity 2017.2. Additionally, the past review discussion suggested a more defensive approach with fallbacks (tryCoreModule→ tryUnityEngine→ scan loaded assemblies), but the current implementation only probesCoreModule.While this should work for Unity 2019+ (your target), consider whether the more defensive approach is warranted for robustness.
If you want to add the defensive fallbacks discussed in the previous review, here's the implementation:
private static Type GetUnityApplication() { try { - // Attempt to load UnityEngine.Application type at runtime - // The UnityEngine.Application class has been located in the UnityEngine.CoreModule assembly since Unity 2017.1 - // Returns null if the class is not found in UnityEngine.CoreModule - return Type.GetType("UnityEngine.Application, UnityEngine.CoreModule", throwOnError: false); + // 1. Try Unity 2019+ (CoreModule) - introduced in Unity 2017.2 + var application = Type.GetType("UnityEngine.Application, UnityEngine.CoreModule", throwOnError: false); + + if (application != null) + { + return application; + } + + // 2. Try legacy Unity (pre-2017.2 monolithic UnityEngine) + application = Type.GetType("UnityEngine.Application, UnityEngine", throwOnError: false); + + if (application != null) + { + return application; + } + + // 3. Fallback: scan all loaded UnityEngine assemblies + foreach (var assembly in AppDomain.CurrentDomain.GetAssemblies()) + { + var name = assembly.GetName().Name; + if (name != null && name.StartsWith("UnityEngine", StringComparison.Ordinal)) + { + var type = assembly.GetType("UnityEngine.Application"); + if (type != null) + { + return type; + } + } + } + + return null; } catch { // Silently fail - Unity is not available return null; } }
78-101: LGTM! Safe string property access.The method correctly uses the null-conditional operator and safe cast (
as string) to handle potential reflection failures gracefully. The implementation is thread-safe when used with theLazy<string>wrapper.
107-131: LGTM! Safe enum-to-string conversion.The method uses
ToString()to convert theRuntimePlatformenum to its string representation (e.g., "OSXEditor", "WindowsPlayer"). This approach avoids the boxing/unboxing issues that would occur with integer conversion and is safer than the int-based approach discussed in past reviews. The null-conditional operator and null coalescing ensure graceful degradation.src/IO.Ably.Shared/IO.Ably.Shared.projitems (1)
181-182: LGTM! Unity integration files correctly added to build.The two new Unity helper files are properly included in the MSBuild project items, making them available across all project configurations.
unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs (2)
10-10: LGTM! Necessary using directive for Unity helpers.The
using IO.Ably.Unitydirective is required to accessUnityHelper.UnityIdentifierused in the test assertions.
212-246: LGTM! Test correctly migrated to NUnit and property-based access.The test method has been properly updated to:
- Use NUnit assertions instead of FluentAssertions (enables play mode testing on devices per PR objectives)
- Access
Agent.DotnetRuntimeIdentifier,Agent.OsIdentifier, andUnityHelper.UnityIdentifieras properties (matching the Lazy-cached property pattern introduced in the Agent and Unity helpers)- Maintain correct assertion logic for Unity agent header validation
src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
98-150: LGTM! Comprehensive platform detection logic.The
GetOsIdentifier()method correctly maps all Unity runtime platforms to their corresponding Ably agent identifiers. The try-catch block provides graceful degradation for unknown future Unity platforms, and the empty string return aligns with the non-Unity fallback behavior.
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the Unity integration by removing external dependencies (FluentAssertions, separate DLL files), implementing reflection-based Unity detection to eliminate compile-time dependencies, and updating documentation to reflect simplified setup requirements. The changes consolidate all dependencies into a single IO.Ably.dll and replace assertion libraries with NUnit in tests.
- Replaces FluentAssertions with NUnit assertions across all test files
- Implements reflection-based Unity platform detection via new
UnityAdapterandUnityHelperclasses - Consolidates multiple DLLs into single merged
IO.Ably.dllfor Unity - Removes
AutomaticNetworkStateMonitoring = falserequirement from Unity setup
Reviewed Changes
Copilot reviewed 45 out of 71 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| unity/README.md | Simplified Unity setup requirements and updated example references |
| unity/CONTRIBUTING.md | Removed assembly validation documentation and updated plugin update process |
| unity/Assets/Tests/**/*.cs | Converted FluentAssertions to NUnit assertions and renamed test classes |
| src/IO.Ably.Shared/Agent.cs | Refactored to use properties instead of methods and integrated UnityHelper |
| src/IO.Ably.Shared/Unity/*.cs | New reflection-based Unity detection without compile-time dependencies |
| cake-build/tasks/build.cake | Added DLL merging task for Unity package creation |
| unity/Assets/Ably/Plugins/link.xml | Updated to preserve UnityEngine.CoreModule properties |
Comments suppressed due to low confidence (2)
unity/Assets/Tests/PlayMode/AuthSpecs.cs:612
- These time-based assertions are fragile. The assertions compare
token.Issuedandtoken.ExpirestoDateTimeOffset.UtcNowwhich is evaluated at assertion time, not at token creation time. This creates a race condition where the test could fail if there's any delay between token creation and assertion. Consider capturingDateTimeOffset.UtcNowbefore the token request and using that captured value for assertions.
unity/Assets/Tests/EditMode/AuthSpecs.cs:611 - These time-based assertions are fragile. The assertions compare
token.Issuedandtoken.ExpirestoDateTimeOffset.UtcNowwhich is evaluated at assertion time, not at token creation time. This creates a race condition where the test could fail if there's any delay between token creation and assertion. Consider capturingDateTimeOffset.UtcNowbefore the token request and using that captured value for assertions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added cake script to build IO.Ably.Dll with all dependencies merged - Removed old dependencies from Ably/Plugins - Updated README and CONTRIBUTING guide
- Updated test assertions to support native NUnit asserts - Updated PlayMode.asmdef to exclude WebGL
- Removed references to UnityEngine imports, used reflection based approach - Improved agent header caching machanism in Agent.cs - Created UnityAdapter and UnityHelper to access unity specific APIs
- Updated UnityAdapter to handle instance access for different unity versions - Fixed unity editmode and playmode tests for agent identifiers - Updated link.xml with exclusions for needed types
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
9459200 to
9a7ea90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
unity/Assets/Ably/Plugins/link.xml (1)
3-8: Address the contradiction regarding UnityEngine.Application preservation.This adds explicit link.xml directives to preserve
UnityEngine.Application.unityVersionandUnityEngine.Application.platform, but a past review flagged a contradictory comment in UnityAdapter.cs stating these directives are unnecessary because "UnityEngine.Application (it's always preserved)".Clarify whether:
- These properties are always preserved by Unity's il2cpp linker (in which case the link.xml entries are redundant and the comment is correct), or
- These properties are not always preserved (in which case the link.xml entries are necessary and the comment needs updating).
Please verify whether these properties are actually used in the codebase and whether explicit preservation is necessary:
#!/bin/bash # Description: Find usages of UnityEngine.Application.unityVersion and platform in the codebase # Expected: Show all call sites accessing these properties to confirm they are actually used rg -nP --type=cs '\bApplication\.(unityVersion|platform)\b' unity/Assets/Ably/ rg -nP --type=cs 'GetType.*Application' unity/Assets/Ably/ -A 3 -B 3src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
156-165: Fix incorrect Unity identifier when Unity is unavailable.This issue was previously flagged: When
UnityAdapter.IsAvailableisfalse(line 160), the method returns"unity", which incorrectly reports Unity presence in the agent header even when running outside Unity. This should return an empty string to exclude the Unity identifier from the agent header when Unity is not available.Apply this fix:
private static string GetUnityIdentifier() { if (!UnityAdapter.IsAvailable) { - return "unity"; + return string.Empty; } var version = UnityAdapter.UnityVersion; return version.IsEmpty() ? "unity" : $"unity/{version}"; }This ensures the Unity identifier is only included when actually running in Unity, with a fallback to
"unity"(without version) if the version string cannot be determined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (15)
lib/UnityEngine.dllis excluded by!**/*.dlllib/unity/AOT/Newtonsoft.Json.dllis excluded by!**/*.dllunity/Assets/Ably/Examples/Dashboard/AblyDashboard.unityis excluded by!**/*.unityunity/Assets/Ably/Examples/Dashboard/logo.pngis excluded by!**/*.pngunity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/IO.Ably.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Threading.Channels.dllis excluded by!**/*.dllunity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dllis excluded by!**/*.dllunity/ProjectSettings/EditorBuildSettings.assetis excluded by!**/*.assetunity/ProjectSettings/PackageManagerSettings.assetis excluded by!**/*.assetunity/ProjectSettings/ProjectSettings.assetis excluded by!**/*.assetunity/doc_images/assembly_version_validation.PNGis excluded by!**/*.png
📒 Files selected for processing (42)
README.md(1 hunks)cake-build/tasks/build.cake(2 hunks)lib/unity/AOT/Newtonsoft.Json.dll.meta(1 hunks)src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj(1 hunks)src/IO.Ably.Shared/Agent.cs(5 hunks)src/IO.Ably.Shared/IO.Ably.Shared.projitems(1 hunks)src/IO.Ably.Shared/Unity/UnityAdapter.cs(1 hunks)src/IO.Ably.Shared/Unity/UnityHelper.cs(1 hunks)src/IO.Ably.Tests.Shared/AgentTests.cs(1 hunks)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs(2 hunks)src/IO.Ably.Tests.Shared/Rest/AblyHttpClientSpecs.cs(2 hunks)unity-plugins-updater.cmd(1 hunks)unity-plugins-updater.sh(1 hunks)unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs(2 hunks)unity/Assets/Ably/Examples/Dashboard/AblyMain.cs(3 hunks)unity/Assets/Ably/Examples/Dashboard/AblyPresence.cs(2 hunks)unity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dll.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.xml.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dll.meta(0 hunks)unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.xml.meta(0 hunks)unity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dll.meta(0 hunks)unity/Assets/Ably/Plugins/System.Threading.Channels.dll.meta(0 hunks)unity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dll.meta(0 hunks)unity/Assets/Ably/Plugins/link.xml(1 hunks)unity/Assets/Tests/AblySandbox/AblyRealtimeExtensions.cs(0 hunks)unity/Assets/Tests/AblySandbox/AblySandbox.asmdef(1 hunks)unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs(2 hunks)unity/Assets/Tests/EditMode/AuthSpecs.cs(32 hunks)unity/Assets/Tests/EditMode/EditMode.asmdef(1 hunks)unity/Assets/Tests/EditMode/StatsSpecs.cs(2 hunks)unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs(3 hunks)unity/Assets/Tests/PlayMode/AuthSpecs.cs(32 hunks)unity/Assets/Tests/PlayMode/PlayMode.asmdef(1 hunks)unity/Assets/Tests/PlayMode/StatsSpecs.cs(2 hunks)unity/CONTRIBUTING.md(1 hunks)unity/Packages/manifest.json(1 hunks)unity/Packages/packages-lock.json(0 hunks)unity/README.md(2 hunks)
💤 Files with no reviewable changes (13)
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.xml.meta
- unity/Assets/Tests/AblySandbox/AblyRealtimeExtensions.cs
- unity/Assets/Ably/Plugins/System.Threading.Channels.dll.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.xml.meta
- unity/Assets/Ably/Plugins/System.Runtime.CompilerServices.Unsafe.dll.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT/Newtonsoft.Json.deps.json
- unity/Assets/Ably/Plugins/NewtonsoftJsonAOT.meta
- unity/Packages/packages-lock.json
- unity/Assets/Ably/Plugins/IO.Ably.DeltaCodec.dll.meta
- unity/Assets/Ably/Plugins/System.Threading.Tasks.Extensions.dll.meta
- unity/Assets/Ably/Plugins/NewtonsoftJsonEditor/Newtonsoft.Json.dll.meta
🚧 Files skipped from review as they are similar to previous changes (16)
- lib/unity/AOT/Newtonsoft.Json.dll.meta
- unity/Assets/Tests/PlayMode/StatsSpecs.cs
- src/IO.Ably.Shared/Unity/UnityAdapter.cs
- unity/Packages/manifest.json
- src/IO.Ably.Shared/IO.Ably.Shared.projitems
- unity/Assets/Tests/EditMode/EditMode.asmdef
- cake-build/tasks/build.cake
- README.md
- src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj
- unity/Assets/Tests/EditMode/StatsSpecs.cs
- src/IO.Ably.Tests.Shared/AgentTests.cs
- unity/Assets/Tests/PlayMode/PlayMode.asmdef
- src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionParameterSpecs.cs
- unity/README.md
- unity/Assets/Tests/AblySandbox/AblySandbox.asmdef
- unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs
🧰 Additional context used
🧬 Code graph analysis (8)
unity/Assets/Tests/EditMode/AuthSpecs.cs (3)
src/IO.Ably.Shared/TokenDetails.cs (1)
IsValidToken(131-146)src/IO.Ably.Shared/Types/ErrorCodes.cs (1)
ErrorCodes(3-111)unity/Assets/Tests/PlayMode/AuthSpecs.cs (5)
Task(1014-1042)Task(1044-1067)Task(1069-1075)RSA4Helper(1002-1076)RSA4Helper(1008-1012)
unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
UnityHelper(9-166)
unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (1)
UnityHelper(9-166)
unity/Assets/Tests/PlayMode/AuthSpecs.cs (3)
src/IO.Ably.Shared/TokenDetails.cs (1)
IsValidToken(131-146)src/IO.Ably.Shared/Types/ErrorCodes.cs (1)
ErrorCodes(3-111)unity/Assets/Tests/EditMode/AuthSpecs.cs (5)
Task(1013-1041)Task(1043-1066)Task(1068-1074)RSA4Helper(1001-1075)RSA4Helper(1007-1011)
src/IO.Ably.Tests.Shared/Rest/AblyHttpClientSpecs.cs (2)
src/IO.Ably.Shared/Agent.cs (1)
Agent(11-245)src/IO.Ably.NETStandard20/Platform.cs (1)
Agent(28-52)
src/IO.Ably.Shared/Agent.cs (1)
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
GetOsIdentifier(98-150)UnityHelper(9-166)
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
src/IO.Ably.Shared/Agent.cs (1)
GetOsIdentifier(107-206)src/IO.Ably.Shared/Unity/UnityAdapter.cs (1)
UnityAdapter(26-132)
unity/Assets/Ably/Examples/Dashboard/AblyMain.cs (2)
unity/Assets/Ably/Examples/Dashboard/AblyChannel.cs (4)
AblyChannel(8-123)AblyChannel(22-26)AblyChannel(28-31)RegisterUiComponents(33-54)unity/Assets/Ably/Examples/Dashboard/AblyPresence.cs (4)
AblyPresence(7-142)AblyPresence(21-25)AblyPresence(27-30)RegisterUiComponents(32-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
🔇 Additional comments (11)
unity/CONTRIBUTING.md (1)
151-154: Documentation accurately reflects the implemented behavior.Verification confirms that the documentation at line 154 is accurate. The updater scripts call Cake build targets that invoke ILRepack to merge dependencies into
IO.Ably.dll(confirmed incake-build/tasks/build.cakeline 111) and output it tounity/Assets/Ably/Plugins/(confirmed incake-build/tasks/build.cakeline 147). The changes clarify the script's behavior and align with the PR objective to create a unified merged DLL for Unity.unity-plugins-updater.sh (1)
6-7: Good consistency with the CMD script, but verify dependencies and consider error handling.The shell script now mirrors the Windows CMD workflow, which is excellent for maintainability. However, the same verification about the
Update.AblyUnity→Build.NetStandarddependency chain applies here (see comments onunity-plugins-updater.cmd).For Unix shells, consider adding
set -eat the beginning of the script to fail fast on any error:+set -e + if [ $# -eq 0 ] then echo "Provide latest version number like unity-plugins-updater.sh 1.2.8"Alternatively, add explicit error checking after each command:
dotnet tool restore -dotnet cake cake-build/build.cake -- --target=Build.NetStandard --define=UNITY_PACKAGE -dotnet cake cake-build/build.cake -- --target=Update.AblyUnity +dotnet cake cake-build/build.cake -- --target=Build.NetStandard --define=UNITY_PACKAGE || exit 1 +dotnet cake cake-build/build.cake -- --target=Update.AblyUnity || exit 1unity-plugins-updater.cmd (1)
4-5: Dependency chain verified—code is correct as written.The
Update.AblyUnitytarget depends on_Build_Ably_Unity_Dll(cake-build/tasks/build.cake:190-192), which explicitly validates the existence ofIO.Ably.NETStandard20/bin/Release/netstandard2.0/IO.Ably.dlland throws exceptions with clear messages if the build hasn't completed first. The sequential invocation ofBuild.NetStandardfollowed byUpdate.AblyUnityis intentional and necessary—runningUpdate.AblyUnitystandalone will fail with informative errors requiring the prior build step. The dependency chain is sound through explicit runtime validation rather than Cake task dependencies. Error handling with|| exit /b 1is optional but not required for correctness.unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs (2)
10-10: LGTM! Correct namespace import and improved test class naming.The addition of
IO.Ably.Unitynamespace enables access toUnityHelper, and the class rename toAblyRealtimeSpecsbetter reflects the test scope.Also applies to: 17-17
222-243: LGTM! Correct migration to NUnit assertions and property-based access.The test correctly:
- Replaces FluentAssertions with NUnit assertions
- Updates method calls to property access (
Agent.DotnetRuntimeIdentifier,Agent.OsIdentifier)- Uses
UnityHelper.UnityIdentifierinstead of the removedAgent.UnityPlayerIdentifier()These changes align with the PR's objective to remove FluentAssertions and refactor Agent identifiers to cached properties.
src/IO.Ably.Shared/Unity/UnityHelper.cs (2)
11-15: LGTM! Well-designed lazy initialization and platform detection constants.The implementation correctly:
- Uses lazy initialization to cache identifiers, avoiding repeated reflection calls
- Defines platform-specific constants matching Ably's agent protocol format
- Provides a
RuntimePlatformnested class with string constants that mirror UnityEngine enum values without requiring a compile-time dependencyThis design enables Unity detection without directly referencing UnityEngine assemblies.
Also applies to: 19-29, 36-79
85-85: LGTM! Correct property exposure and OS detection logic.The properties correctly expose lazy-cached values, and
GetOsIdentifierproperly:
- Returns empty string when Unity is unavailable
- Maps platform strings to Ably-compatible identifiers
- Handles unknown platforms gracefully with a catch-all fallback
The generic catch clause (lines 144-146) could optionally specify
Exceptiontype for clarity, as suggested in past reviews, but this is a minor style point.Also applies to: 91-91, 98-150
src/IO.Ably.Shared/Agent.cs (4)
39-55: LGTM! Excellent refactoring to lazy-cached properties.The addition of lazy-initialized fields and their corresponding properties improves performance by computing runtime and OS identifiers once and caching the results. The clear documentation and consistent pattern align well with the
UnityHelperimplementation.
64-64: LGTM! Correct method renames for private lazy initialization.The methods are properly renamed to
GetDotnetRuntimeIdentifier()andGetOsIdentifier(), following the naming convention for private initializers. Public access is correctly provided through the lazy-cached properties.Also applies to: 107-107
117-119: LGTM! Unity OS identifier integration is correct.The direct return of
UnityHelper.OsIdentifierunderUNITY_PACKAGEis appropriate. When Unity cannot detect the platform, it returns an empty string, which aligns with this method's fallback behavior at line 204. The past review concern about this approach was reasonably addressed by the developer.
225-225: LGTM! Correct property usage in agent composition.The agent composition correctly uses:
DotnetRuntimeIdentifierandOsIdentifierproperties instead of method callsUnityHelper.UnityIdentifierinstead of the removedAgent.UnityPlayerIdentifier()These changes align with the refactoring to lazy-cached properties and Unity integration through
UnityHelper.Also applies to: 228-228, 231-231
lib/unity/AOT.IO.Ably.dllwith all merged dependencies.FluentAssertionsdependency to allow running tests in play mode ( actual device ).Summary by CodeRabbit
New Features
Improvements
Chores
Documentation
Tests