Unity native variables sync #160
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces multiple improvements across the codebase. UI behavior is adjusted by removing a conditional disable in the navigation menu, while variable handling is enhanced with additional null checks and conditional logic. New utility methods for type determination and flattening nested dictionaries have been added, alongside new classes and methods to support event queue management and variable synchronization. Enhanced error logging and detailed network response handling are also present, with new tests ensuring the expected behavior of the variable utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Wrapper as UnityNativeWrapper
participant Manager as UnityNativeEventManager
participant QueueMgr as UnityNativeEventQueueManager
participant SingleQ as UnityNativeSingleEventQueue
participant Network as UnityNativeNetworkEngine
Wrapper->>Manager: SyncVariables(varsPayload)
Manager->>QueueMgr: Create DefineVarsEvent with payload
QueueMgr->>SingleQ: Enqueue DefineVarsEvent
SingleQ->>QueueMgr: [Timer Tick] Trigger Flush
QueueMgr->>Network: SendRequest(event)
Network-->>QueueMgr: Response
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs (3)
14-14: Reconfirm the constant queue limit.
HardcodingQUEUE_LIMIT = 1is consistent with this "single event" logic, but if there is any possibility of extending this queue in the future, consider making it configurable.
91-92: Fix the spelling in the success message.
"Varibles" should be "Variables" to maintain professional log output.- CleverTapLogger.Log("Varibles Successfully synced"); + CleverTapLogger.Log("Variables successfully synced");
116-127: Consider handling or awaiting the FlushEvents task.
The call toFlushEvents()is invoked with_ = ..., discarding the returnedTask. If an exception occurs, it might be lost. Consider awaiting this call or adding error handling to catch potential failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CTExample/Assets/Scripts/NavigationMenu.cs(0 hunks)CTExample/Assets/Scripts/Variables/Variables.cs(2 hunks)CleverTap/Runtime/Common/CleverTapPlatformVariable.cs(1 hunks)CleverTap/Runtime/Common/VariableFactory.cs(1 hunks)CleverTap/Runtime/Constants/CleverTapVariableKind.cs(1 hunks)CleverTap/Runtime/Native/UnityNativePlatformBinding.cs(1 hunks)CleverTap/Runtime/Native/UnityNativePlatformVariable.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/Enums/UnityNativeEventType.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/EventBuilders/UnityNativeBaseEventBuilder.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs.meta(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventManager.cs(2 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs(3 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeNetworkEngine.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeVarCache.cs(2 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeVariableUtils.cs(3 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeWrapper.cs(2 hunks)CleverTap/Tests/Runtime/UnityNativeVariableUtilsTest.cs(2 hunks)
💤 Files with no reviewable changes (1)
- CTExample/Assets/Scripts/NavigationMenu.cs
🔇 Additional comments (39)
CleverTap/Runtime/Native/UnityNativeWrapper/Enums/UnityNativeEventType.cs (1)
13-14: LGTM - Event type enum additionThe addition of
DefineVarsEventto the enum follows the existing pattern of sequential numbering and maintains code consistency.CleverTap/Runtime/Constants/CleverTapVariableKind.cs (1)
1-9: Code documentation improvementGood addition of XML documentation comments that clearly explain the purpose and usage of the CleverTapVariableKind class. The namespace reformatting also improves code readability.
CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs.meta (1)
1-11: Auto-generated Unity meta fileThis is a standard Unity-generated meta file for the new UnityNativeSingleEventQueue class. It contains the necessary Unity asset metadata and doesn't require modifications.
CleverTap/Runtime/Native/UnityNativePlatformBinding.cs (1)
9-10: Appropriate accessor added for internal useAdding this property allows other components to access the UnityNativeWrapper instance while maintaining proper encapsulation. This change enables the new variable synchronization functionality without exposing the wrapper instance beyond its needed scope.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeWrapper.cs (1)
45-49: Good implementation of variable synchronization methodThis method properly delegates to the event manager's SyncVariables method, enabling variable synchronization functionality without duplicating logic. The method signature correctly accepts a dictionary payload that can contain all necessary variable data.
CTExample/Assets/Scripts/Variables/Variables.cs (4)
155-156: Good null check added to prevent potential exceptionsAdding this null check prevents a NullReferenceException when attempting to subscribe to the OnFileReady event if factory_var_file is null. This is a good defensive programming practice.
164-169: Platform-specific implementation for variable synchronizationThe conditional compilation directive ensures that iOS platforms use a different synchronization method (with the force parameter set to true) compared to other platforms. This addresses platform-specific requirements for variable synchronization.
225-228: Improved null handling with null-conditional operatorsUsing the null-conditional operator (?.) is a good approach to prevent null reference exceptions when accessing properties of potentially null variables. This makes the code more robust against runtime errors.
233-235: Appropriate validation added to prevent errorsAdding this null check ensures that the method returns early when the name parameter is null or empty, preventing potential issues when attempting to use the name later in the method. This improves the robustness of the code.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeVarCache.cs (3)
1-2: Proper platform-specific code isolationThe preprocessor directive ensures this code only runs on appropriate platforms (not on iOS/Android or when in the Unity Editor). Adding System.Collections namespace is necessary for handling collections properly.
137-140: Well-implemented method for variable payload retrievalThis method provides a clean way to get a flattened version of the variable payload by delegating to the UnityNativeVariableUtils helper. This supports the variable synchronization feature and follows good separation of concerns.
142-143: Properly closed preprocessor directiveThe closing preprocessor directive correctly matches the opening directive, ensuring the platform-specific code is properly encapsulated.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeNetworkEngine.cs (2)
372-373: Good improvement to error handling with the null-conditional operatorThe addition of the null-conditional operator
?.when accessingunityWebRequest.downloadHandler.textprevents potential NullReferenceExceptions. Additionally, the response now correctly includes the actual HTTP status code and response headers, providing more detailed error information.
377-377: Enhanced error response with actual HTTP status codeThis change improves error reporting by using the actual HTTP response code rather than defaulting to InternalServerError, which provides more accurate context for debugging issues.
CleverTap/Runtime/Common/VariableFactory.cs (2)
7-7: Added utility namespace importThe addition of the CleverTapSDK.Utilities namespace import allows for using the logger functionality that's used later in the code.
24-30: Added null check and error logging for UnityNativePlatformBindingThis improvement adds proper error handling when the binding isn't of the expected type. The null-conditional operator ensures safe access to UnityNativeWrapper, preventing potential NullReferenceExceptions.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventManager.cs (2)
6-7: Moved imports under the platform-specific directiveMoving these imports under the platform-specific directive ensures they're only included when needed for the targeted platform.
103-115: Added SyncVariables method for variable synchronizationThis new method properly handles the synchronization of variables by:
- Checking if the event should be deferred
- Building an event of type DefineVarsEvent with the payload
- Queuing the event for processing
The implementation follows the established pattern in the codebase for other event types and properly handles deferred events.
CleverTap/Runtime/Common/CleverTapPlatformVariable.cs (2)
145-146: Refactored type checking to call the new GetKindNameFromType methodGood refactoring to call the centralized type checking method that's been added.
148-172: Added centralized type checking method with comprehensive type supportThe new
GetKindNameFromTypemethod centralizes the logic for determining variable types, making the code more maintainable. It properly handles various numeric types, strings, dictionaries, and booleans.This method supports better code reuse and consistency in type determination across the SDK.
CleverTap/Tests/Runtime/UnityNativeVariableUtilsTest.cs (2)
3-7: Appropriate imports added for new test functionality.The additional imports provide necessary access to the CleverTap SDK common classes, constants, and Unity functionality required for the new tests.
365-471: Well-structured test cases for the new GetFlatVarsPayload functionality.The test cases comprehensively cover:
- Various variable types (string, int, dictionary, float)
- Nested dictionary flattening
- Empty dictionary handling
- Proper type conversion from CleverTap variable kinds to payload types
The test assertions are thorough and verify both the structure and content of the output payload. This ensures that variable synchronization with native platforms will work correctly.
CleverTap/Runtime/Native/UnityNativePlatformVariable.cs (5)
4-7: Added necessary Unity import.The addition of Unity's namespace provides access to Debug functionality used for distinguishing between debug and release builds.
11-12: Added dependency for variable synchronization.The
unityNativeWrapperfield enables the class to access native platform capabilities for variable synchronization.
13-16: Good use of dependency injection for testability.The constructor properly initializes the native wrapper, allowing for better testability by injecting dependencies.
19-21: Simplified method now delegates to the overloaded version.The method has been refactored to call the parameterized version with a default value, improving code reuse.
25-56: Enhanced error handling and robust environment checks.The implementation now:
- Verifies build environment (debug vs release) and provides appropriate warnings
- Checks for null references before attempting operations
- Provides clear error messages for different failure scenarios
This helps developers understand when and how to use the SyncVariables method correctly.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs (5)
12-13: Added dedicated queue for single events.This new queue allows for specialized handling of events that should be processed individually, such as variable definitions.
21-22: Proper initialization and event subscription for the new queue.The queue is correctly initialized with core state and network engine, and properly subscribed to the timer tick event.
27-29: Refactored event handling to use centralized method.The method now delegates to the new
QueueEventmethod, improving maintainability and reducing code duplication.
31-47: Well-structured event routing based on event type.The new
QueueEventmethod routes events to the appropriate queue based on event type, including the newDefineVarsEventtype.
63-66: Added timer tick handler for single events queue.This handler follows the established pattern for other event queues, ensuring consistent behavior.
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeVariableUtils.cs (4)
1-7: Appropriate platform-specific compilation directive and imports.The code correctly uses preprocessor directives to exclude the implementation from mobile platforms except in the Unity Editor. The necessary imports have been added for the new functionality.
95-110: Well-implemented dictionary flattening method.The
ConvertNestedDictionariesToFlatmethod recursively processes nested dictionaries, maintaining key hierarchy through dot notation. This creates a flat representation suitable for variable synchronization with native platforms.
112-157: Comprehensive implementation of variable payload creation.The
GetFlatVarsPayloadmethod:
- Properly initializes the payload structure
- Handles different variable types appropriately
- Flattens nested dictionary values
- Applies correct type conversions for each variable
The implementation handles all the complexity of creating a correctly structured payload for native variable synchronization.
257-271: Clean implementation of type conversion using switch expression.The
GetDefineTypeFromKindmethod efficiently maps CleverTap variable kinds to their corresponding payload type strings. The use of a switch expression with pattern matching is concise and readable.CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs (3)
1-9: Conditional compilation block and using directives look good.
No issues spotted with the conditional compilation directives or the using statements.
21-37: Ensure fallback logic is acceptable.
When the queue contains more or fewer events than the limit, this method logs an error and continues with the default record path. Verify that silently falling back toREQUEST_PATH_RECORDis the desired behavior instead of, for instance, throwing an exception or discarding the extra events.
44-59: Response check logic is consistent.
The gating on exactly one event matches the single-event assumption. Logging an error and relying onresponse.IsSuccess()otherwise appears correct.
CleverTap/Runtime/Native/UnityNativeWrapper/EventBuilders/UnityNativeBaseEventBuilder.cs
Show resolved
Hide resolved
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs
Outdated
Show resolved
Hide resolved
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeBaseEventQueue.cs (1)
75-75: Minor spelling fix suggested.
The variable is namedproccesedEvents; consider renaming it toprocessedEventsfor clarity.- var proccesedEvents = new List<UnityNativeEvent>(); + var processedEvents = new List<UnityNativeEvent>();CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs (5)
47-64: NewQueueEventmethod consolidates event handling.
Distinct event types are dispatched to their respective queues. Consider logging an appropriate message in thedefault:case if event types expand in the future.default: + CleverTapLogger.Log($"Unhandled event type: {newEvent.EventType}"); break;
65-70: Use ofasync voidfor flush operations.
async voidcan hide exceptions and complicate error handling. Consider returning aTaskinstead for improved resiliency.-internal async void FlushQueues() +internal async Task FlushQueues()
72-75: Timer callback usesasync void.
Similar best practice considerations apply here. Converting to aTaskreturn type can help ensure proper exception propagation.
77-80: OnRaisedEventTimerTick also declared asasync void.
Consider refactoring toasync Taskand await the caller to handle potential exceptions.
82-85: OnSingleEventTimerTick.
Same rationale:async Taskis safer for asynchronous flow control, enabling external callers to handle errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeBaseEventQueue.cs(6 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeUserEventQueue.cs(1 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeDatabaseStore.cs(4 hunks)CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeUserEventQueue.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeSingleEventQueue.cs
🔇 Additional comments (21)
CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeDatabaseStore.cs (7)
14-14: Method name correction is clear and concise.
RenamingInitilazeDatabasetoInitializeDatabaseimproves overall clarity.
21-35: Mismatch with AI-generated summary.
The summary indicates that the message "Database not intiallised" was changed to "Database not initialized" in theAddEventmethod, but here we still see "intiallised."Likely an incorrect or invalid review comment.
29-29: Conditional event invocation is appropriate.
UsingOnEventStored?.Invoke(storedEvent);is a clean and safe approach.
56-56: Spelling fix for error message is correct.
Changing "Database not intiallised" to "Database not initialized" is consistent.
67-67: Fallback for null event ID.
Storing-1whenevents.Idis null may be functional, but verify that using-1for deletion does not cause other unintended side effects in your system.Would you like me to confirm downstream handling of this fallback value?
75-75: Improved function naming.
Renaming this private method toInitializeDatabaseincreases readability.
83-83: Comment formatting revised.
The clarified comment about creating a table if it does not exist is helpful.CleverTap/Runtime/Native/UnityNativeWrapper/Models/UnityNativeBaseEventQueue.cs (5)
14-14: Delegate addition.
IntroducingEventsProcessedimproves the event-driven approach and makes it easier to handle flushed events downstream.
31-31: New event for event processing.
OnEventsProcessedprovides a valuable callback to notify subscribers when flush operations finish.
114-114: Event callback on error.
InvokingOnEventsProcessedhere ensures consistent notifications even in error scenarios.
136-136: Consistent event invocation.
This helps keep the logic uniform, notifying about processed events after the exception handling block.
151-151: Final flush notification.
This invocation ensures subscribers are informed when the flush concludes, successful or otherwise.CleverTap/Runtime/Native/UnityNativeWrapper/UnityNativeEventQueueManager.cs (9)
2-2: Added namespace import.
ImportingSystem.Collections.Genericaligns with the new usage of lists in this file.
6-8: Namespace and class scope adjustments.
These lines help structure the expanded event queue functionality.
14-14: Single events queue field addition.
The_singleEventsQueuefield supportsDefineVarsEventhandling in a dedicated queue.
16-35: Constructor additions for new queue and subscriptions.
Initializing_singleEventsQueueand subscribing to event callbacks centralizes queue management and deletion flow.
37-40: Database cleanup upon events processed.
Calling_databaseStore.DeleteEvents(flushedEvents);addresses potential data buildup, ensuring a consistent approach across all queues.
42-45: Forwarding newly stored events to the correct queue.
This logic simplifies event routing, ensuring that all events pass throughQueueEvent.
87-91: FlushUserEvents captures user event logic.
No issues found; usage ofTaskreturn helps error handling.
93-97: FlushRaisedEvents is properly asynchronous.
This method also benefits from theTaskapproach, as recommended above.
99-102: FlushSingleEvents method.
Now includes a flush for single events, complementing the changes in the constructor andOnEventsProcessed.
nzagorchev
force-pushed
the
unity_native_variables
branch
2 times, most recently
from
d9c93dc to
5a1a75e
Compare
nzagorchev
force-pushed
the
unity_native_variables_sync
branch
from
39fa71f to
13b9260
Compare
* Merge pull request #153 from CleverTap/setup_tests * Merge pull request #152 from CleverTap/unity_native_variables_define * Merge pull request #156 from CleverTap/unity_native_variables_merge * EventQueue changes (#159) * Unity native variables sync (#160) * Unity Native Variables - Handle Variables responses and Trigger Callbacks (#167) * Unity native variables fetch (#168) * Unity Native Variables - define and sync File variables (#169) * Launch on Unity Native only (#170) * Fix defineVars CORS issue (#174)
Summary by CodeRabbit
New Features
Bug Fixes
These updates improve the reliability and responsiveness of variable interactions and event processing for a smoother user experience.