Release v7.1.0 #399
Release v7.1.0 #399
Conversation
- TODO: Call IsEventFirstTime from the new db/localDataStore class
- Renamed one letter object names to fit the context - TODO: Call appropriate persist method from the new db/localDataStore class
- Added new class CTEventDatabase to store user events in database. - Added create table, insert, update and check event methods.
…ofile-events [MC-2088] Evaluate first time profile events
…ents [MC-2086] Evaluate first time events
- synchronised userEventLogs set access
[MC - 2090] In-memory storage events
[MC-2106] Add event storage limits
[MC-2082] Implement new event storage
[MC-2430] Normalize event names
…e to the database. updated tests. added normalised names to queries.
WalkthroughThe changes introduce version 7.1.0 of the CleverTap SDK, focusing on enhancing event tracking and user-level analytics. A significant addition is the support for first-time event filtering in in-app notifications, allowing more granular campaign triggers. The SDK now includes a new user-level event log tracking system with APIs for retrieving event details, counts, and timestamps. Several device-level event tracking methods have been deprecated in favor of more robust user-specific event tracking approaches. Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
CleverTapSDK/InApps/Matchers/CTTriggersMatcher.h (1)
17-17: LGTM! Well-structured dependency injection.The new initializer follows good architectural practices:
- Uses dependency injection for CTLocalDataStore
- Returns instancetype (preferred over id)
- Enables the new first-time event filtering functionality
This design pattern will make it easier to:
- Unit test the trigger matching logic
- Maintain separation of concerns
- Extend the functionality in the future
CleverTapSDK/InApps/Matchers/CTTriggerAdapter.m (2)
33-33: Add type validation for firstTimeOnly.The
firstTimeOnlyvalue from JSON should be validated to ensure it's of the expected type.Consider adding type validation:
- self.firstTimeOnly = triggerJSON[@"firstTimeOnly"]; + id firstTimeOnly = triggerJSON[@"firstTimeOnly"]; + if ([firstTimeOnly isKindOfClass:[NSString class]]) { + self.firstTimeOnly = firstTimeOnly; + } else { + CleverTapLogStaticDebug(@"Cannot parse firstTimeOnly: %@", firstTimeOnly); + }
33-33: Consider using NSNumber instead of NSString for boolean flag.Since
firstTimeOnlyappears to be a boolean flag based on its name and purpose, consider usingNSNumberinstead ofNSStringfor better type safety and semantic clarity.Update the property type and parsing:
-@property (nonatomic, strong) NSString *firstTimeOnly; +@property (nonatomic, strong) NSNumber *firstTimeOnly; // In initWithJSON: - self.firstTimeOnly = triggerJSON[@"firstTimeOnly"]; + id firstTimeOnly = triggerJSON[@"firstTimeOnly"]; + if ([firstTimeOnly isKindOfClass:[NSNumber class]]) { + self.firstTimeOnly = firstTimeOnly; + } else if ([firstTimeOnly isKindOfClass:[NSString class]]) { + // Handle string values like "true", "false" for backward compatibility + self.firstTimeOnly = @([firstTimeOnly boolValue]); + } else { + CleverTapLogStaticDebug(@"Cannot parse firstTimeOnly: %@", firstTimeOnly); + }CleverTapSDK.xcodeproj/project.pbxproj (1)
Line range hint
1-3200: Overall Project Structure AssessmentThe project structure maintains good organization with:
- Clear separation of source and test files
- Proper grouping of related files
- Correct file ownership and target membership
- Appropriate build phase configurations
- Consistent use of framework search paths and linking
Consider the following recommendations:
- Add a README.md in the EventDatabase folder documenting its purpose and usage
- Consider adding a CHANGELOG.md to track significant changes
- Consider setting up SwiftLint or similar tools for consistent code style
CleverTapSDK/CleverTapEventDetail.h (1)
6-6: Add documentation for new properties.Please add documentation comments for
normalizedEventNameanddeviceIDproperties to explain their purpose and usage in the context of user-level event tracking.Example documentation:
/** * The normalized version of the event name used for consistent tracking. */ @property (nonatomic, strong) NSString *normalizedEventName; /** * The unique identifier of the device associated with this event. */ @property (nonatomic, strong) NSString *deviceID;Also applies to: 10-10
CleverTapSDKTests/CTLocalDataStore+Tests.h (1)
11-14: Add documentation and nullability annotation.Please add:
- Documentation for the test category interface explaining its purpose.
- Nullability annotation for the block parameter.
Example:
NS_ASSUME_NONNULL_BEGIN /** * Test category providing access to internal queue for asynchronous testing. */ @interface CTLocalDataStore (Tests) /** * Executes the given block on the background queue. * @param taskBlock The block to execute. Must not be nil. */ - (void)runOnBackgroundQueue:(nonnull void (^)(void))taskBlock; /** * The background queue used for asynchronous operations. */ @property (nonatomic, readonly) dispatch_queue_t backgroundQueue; @end NS_ASSUME_NONNULL_ENDCleverTapSDK/CTDispatchQueueManager.h (3)
19-19: Add documentation for the new method.The new
inSerialQueuemethod needs documentation to explain its purpose, return value meaning, and thread safety implications.Add a documentation block:
+/** + * Checks if the current execution context is within the serial queue. + * @return YES if the current execution is in the serial queue, NO otherwise + */ - (BOOL)inSerialQueue;
19-19: LGTM! Good addition for thread safety.The new
inSerialQueuemethod is a valuable addition that will help ensure thread-safe access to the new user-level event tracking system.Consider documenting thread safety requirements in the header, especially since this method will likely be used to enforce serial queue access for critical event logging operations.
19-19: Add documentation for the new method.The new
inSerialQueuemethod looks good, but please add documentation to explain its purpose and usage. This is especially important as it may be used for ensuring thread-safety in the new event tracking system.Add a documentation block like:
+/** + * Checks if the current execution context is within the serial queue. + * @return YES if running in the serial queue, NO otherwise + */ - (BOOL)inSerialQueue;CleverTapSDKTests/EventDatabase/CTEventDatabase+Tests.h (2)
16-17: Add nullability annotations and documentation.The initializer method should specify nullability requirements and include documentation explaining its testing purpose.
Add annotations and documentation:
+/** + * Test-specific initializer that allows injection of dependencies. + * @param dispatchQueueManager The queue manager to use for test operations + * @param clock The clock implementation for controlling time in tests + * @return A new instance of CTEventDatabase configured for testing + */ - - (instancetype)initWithDispatchQueueManager:(CTDispatchQueueManager*)dispatchQueueManager - clock:(id<CTClock>)clock; + - (instancetype _Nonnull)initWithDispatchQueueManager:(CTDispatchQueueManager* _Nonnull)dispatchQueueManager + clock:(id<CTClock> _Nonnull)clock;
14-18: LGTM! Well-structured test category.The test category provides good dependency injection support for testing the new event logging system.
Consider adding documentation comments to explain:
- The purpose of this test category
- Expected usage in test scenarios
- Requirements for the clock protocol implementation
CleverTapSDKTests/InApps/InAppHelper.h (2)
19-19: Document the dataStore property and consider property grouping.The new dataStore property should be documented and grouped with related properties for better organization.
Add documentation and consider reordering:
@class CTLocalDataStore; // ... other code ... +/** Local data store for managing user event data in tests */ @property (nonatomic, strong) CTLocalDataStore *dataStore;Consider grouping this property with other storage-related properties if any exist in the class.
Also applies to: 39-39
19-19: Document the dataStore property's purpose.The integration of CTLocalDataStore looks good and aligns with the PR's objective of supporting first-time event filtering. Please add documentation to explain the property's role in the test infrastructure.
Add a documentation block like:
+/** + * Local data store instance used for managing event data in tests. + * This supports testing of first-time event filtering functionality. + */ @property (nonatomic, strong) CTLocalDataStore *dataStore;Also applies to: 39-39
CleverTapSDK/EventDatabase/CTEventDatabase.h (1)
30-32: Consider adding completion handler for upsertEvent method.For consistency with other async operations and to handle potential database errors, consider adding a completion handler to the upsertEvent method.
- (void)upsertEvent:(NSString *)eventName normalizedEventName:(NSString *)normalizedEventName deviceID:(NSString *)deviceID; +- (void)upsertEvent:(NSString *)eventName +normalizedEventName:(NSString *)normalizedEventName + deviceID:(NSString *)deviceID + completion:(void (^)(BOOL success))completion;CleverTapSDK/CTLocalDataStore.h (1)
49-55: Consider adding nullability annotations.The new user-level event tracking methods should specify nullability for parameters and return types to improve Swift interoperability.
-- (BOOL)isEventLoggedFirstTime:(NSString*)eventName; +- (BOOL)isEventLoggedFirstTime:(NSString * _Nonnull)eventName; -- (int)readUserEventLogCount:(NSString *)eventName; +- (int)readUserEventLogCount:(NSString * _Nonnull)eventName; -- (CleverTapEventDetail *)readUserEventLog:(NSString *)eventName; +- (nullable CleverTapEventDetail *)readUserEventLog:(NSString * _Nonnull)eventName; -- (NSDictionary *)readUserEventLogs; +- (NSDictionary * _Nonnull)readUserEventLogs;CleverTapSDKTests/CTLocalDataStoreTests.m (3)
114-120: Add edge cases to profile value test.Consider adding tests for:
- Setting null values
- Setting invalid types
- Updating existing values
123-127: Add validation to profile field test.Consider adding assertions for:
- Key validation
- Value type validation
- Error cases
129-154: Enhance event persistence test coverage.The test verifies basic functionality but should also test:
- Multiple events persistence
- Event data validation
- Error handling
- Event count limits
CleverTapSDKTests/InApps/CTTriggersMatcherTest.m (1)
2013-2099: Well-structured test cases for first-time event filtering!The test cases effectively cover the core functionality of first-time event filtering with proper mock usage and verifications.
Consider adding these test cases to improve coverage:
- Negative test case where
isEventLoggedFirstTimereturnsNO- Edge case where
firstTimeOnlyisNOor not present- Edge case where
firstTimeOnlyhas an invalid valueExample of a negative test case:
+ - (void)testMatchEventWithFirstTimeOnlyWhenNotFirstTime { + NSArray *whenTriggers = @[ + @{ + @"eventName": @"event1", + @"firstTimeOnly": @YES + } + ]; + + CTTriggersMatcher *triggerMatcher = [[CTTriggersMatcher alloc] initWithDataStore:self.dataStore]; + CTLocalDataStore *dataStoreMock = OCMPartialMock(self.dataStore); + id mockIsEventLoggedFirstTime = OCMStub([dataStoreMock isEventLoggedFirstTime:@"event1"]).andReturn(NO); + + BOOL match = [triggerMatcher matchEventWhenTriggers:whenTriggers eventName:@"event1" eventProperties:@{}]; + + XCTAssertFalse(match); + OCMVerify(mockIsEventLoggedFirstTime); + }CleverTapSDK/CleverTap.m (1)
3115-3142: Review new user event log tracking methods.The implementation looks good but consider adding documentation comments to describe:
- The return value semantics (e.g., what does -1 mean for
getUserEventLogCount)- Whether these methods are thread-safe
- Performance characteristics for large event histories
+/** + * Gets the number of times a specific event occurred for the current user. + * @param eventName The name of the event to count + * @return Count of event occurrences or -1 if personalization is disabled + * @note This method is thread-safe and can be called from any thread + */ - (int)getUserEventLogCount:(NSString *)eventName { if (!self.config.enablePersonalization) { return -1; } return [self.localDataStore readUserEventLogCount:eventName]; }CHANGELOG.md (1)
4-4: Update the release date to 2025.The release date should be January 21, 2025 to match the current date.
-### [Version 7.1.0](https://github.com/CleverTap/clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) +### [Version 7.1.0](https://github.com/CleverTap/clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2025)🧰 Tools
🪛 LanguageTool
[inconsistency] ~4-~4: A new year has begun. Did you mean “January 21, 2025”?
Context: .../clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) #### Added - Adds support for trigger...(DATE_NEW_YEAR)
🪛 Markdownlint (0.37.0)
4-4: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
CHANGELOG.md(1 hunks)CleverTap-iOS-SDK.podspec(1 hunks)CleverTapSDK.xcodeproj/project.pbxproj(19 hunks)CleverTapSDK/CTConstants.h(1 hunks)CleverTapSDK/CTDispatchQueueManager.h(1 hunks)CleverTapSDK/CTLocalDataStore.h(2 hunks)CleverTapSDK/CTLocalDataStore.m(10 hunks)CleverTapSDK/CleverTap.h(7 hunks)CleverTapSDK/CleverTap.m(6 hunks)CleverTapSDK/CleverTapBuildInfo.h(1 hunks)CleverTapSDK/CleverTapEventDetail.h(1 hunks)CleverTapSDK/CleverTapEventDetail.m(1 hunks)CleverTapSDK/EventDatabase/CTEventDatabase.h(1 hunks)CleverTapSDK/EventDatabase/CTEventDatabase.m(1 hunks)CleverTapSDK/InApps/CTInAppEvaluationManager.h(2 hunks)CleverTapSDK/InApps/CTInAppEvaluationManager.m(2 hunks)CleverTapSDK/InApps/Matchers/CTTriggerAdapter.h(1 hunks)CleverTapSDK/InApps/Matchers/CTTriggerAdapter.m(1 hunks)CleverTapSDK/InApps/Matchers/CTTriggersMatcher.h(1 hunks)CleverTapSDK/InApps/Matchers/CTTriggersMatcher.m(3 hunks)CleverTapSDKTests/CTLocalDataStore+Tests.h(1 hunks)CleverTapSDKTests/CTLocalDataStore+Tests.m(1 hunks)CleverTapSDKTests/CTLocalDataStoreTests.m(2 hunks)CleverTapSDKTests/EventDatabase/CTEventDatabase+Tests.h(1 hunks)CleverTapSDKTests/EventDatabase/CTEventDatabaseTests.m(1 hunks)CleverTapSDKTests/InApps/CTInAppEvaluationManagerTest.m(1 hunks)CleverTapSDKTests/InApps/CTTriggersMatcherTest.m(56 hunks)CleverTapSDKTests/InApps/InAppHelper.h(2 hunks)CleverTapSDKTests/InApps/InAppHelper.m(2 hunks)Package.swift(1 hunks)sdk-version.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sdk-version.txt
- CleverTapSDK/CleverTapBuildInfo.h
👮 Files not reviewed due to content moderation or server errors (8)
- CleverTapSDK/CTDispatchQueueManager.h
- CleverTapSDKTests/EventDatabase/CTEventDatabase+Tests.h
- CleverTapSDKTests/InApps/InAppHelper.h
- CleverTapSDK/InApps/CTInAppEvaluationManager.h
- CleverTapSDKTests/EventDatabase/CTEventDatabaseTests.m
- CleverTapSDK/InApps/CTInAppEvaluationManager.m
- CleverTapSDKTests/InApps/CTInAppEvaluationManagerTest.m
- CleverTapSDK/CleverTap.h
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[inconsistency] ~4-~4: A new year has begun. Did you mean “January 21, 2025”?
Context: .../clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) #### Added - Adds support for trigger...
(DATE_NEW_YEAR)
[grammar] ~6-~6: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ses/tag/7.1.0) (January 21, 2024) #### Added - Adds support for triggering InApps based on ...
(REPEATED_VERBS)
🪛 Markdownlint (0.37.0)
CHANGELOG.md
4-4: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (41)
CleverTapSDK/InApps/Matchers/CTTriggersMatcher.h (1)
11-11: LGTM! Clean dependency management.The explicit import of CTLocalDataStore.h is appropriate as it's required for the new user-level event tracking functionality.
CleverTapSDK/InApps/Matchers/CTTriggerAdapter.m (1)
33-33: Verify integration with CTTriggersMatcher.The
firstTimeOnlyproperty is used by theCTTriggersMatcherclass to determine if an event should only trigger on its first occurrence. Let's verify this integration.✅ Verification successful
Integration verified and working as expected
The
firstTimeOnlyproperty is properly integrated:
- Correctly declared in adapter
- Used appropriately in CTTriggersMatcher for first-time event validation
- Well-tested with multiple test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of firstTimeOnly in CTTriggersMatcher echo "Searching for firstTimeOnly usage in CTTriggersMatcher..." rg -A 5 "firstTimeOnly" --type objc # Search for related method implementations echo "Searching for matchFirstTimeOnlyForTrigger method..." ast-grep --pattern 'matchFirstTimeOnlyForTrigger:$_'Length of output: 3303
CleverTapSDK/InApps/Matchers/CTTriggerAdapter.h (2)
26-26: Verify thread safety of the firstTimeOnly property.Since this property affects trigger behavior and could potentially be accessed from multiple threads when evaluating in-app campaign conditions, we should ensure proper thread safety.
✅ Verification successful
Property is thread-safe in current implementation
The
firstTimeOnlyproperty is thread-safe because:
- It's immutable after initialization (set only once from JSON)
- Core operations run on serial queues
- Nonatomic property provides atomic access
Suggestions for improved clarity:
- Consider making it readonly in public interface
- Add thread safety documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns of thread/queue usage in trigger evaluation rg -A 5 "firstTimeOnly" --type objc rg -A 5 "dispatch_[a-z_]+" --type objcLength of output: 55987
Line range hint
13-15: Verify JSON parsing for the new property.Since the class is initialized with JSON data, we should ensure proper handling of the
firstTimeOnlyproperty during initialization.✅ Verification successful
JSON parsing for
firstTimeOnlyproperty is correctly implementedThe property is properly initialized from JSON in
initWithJSON:method following the same pattern as other properties in the class. The implementation handles both the presence and absence of the property in JSON data.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the initialization method implementation ast-grep --pattern 'initWithJSON:$_' # Look for JSON schema or documentation fd -e json -e md . | xargs rg -l "firstTimeOnly"Length of output: 17670
Script:
#!/bin/bash # Find the implementation file and look for property declarations and JSON parsing rg -t objc -A 5 "CTTriggerAdapter" # Search for JSON parsing related code rg -t objc "initWithJSON|parseJSON|fromJSON" --context 3 # Look for property declarations ast-grep --pattern '@property $$$firstTimeOnly$$$'Length of output: 59701
Script:
#!/bin/bash # Find property declaration in header rg -t objc "firstTimeOnly" "CleverTapSDK/InApps/Matchers/CTTriggerAdapter.h" # Look for JSON key constant definition rg -t objc "firstTimeOnly|FIRST_TIME_ONLY" "CleverTapSDK/CTConstants.h"Length of output: 196
Script:
#!/bin/bash # Get complete implementation of CTTriggerAdapter.m rg -t objc -A 10 "initWithJSON:" "CleverTapSDK/InApps/Matchers/CTTriggerAdapter.m" # Look for firstTimeOnly initialization rg -t objc "firstTimeOnly.*=" "CleverTapSDK/InApps/Matchers/CTTriggerAdapter.m"Length of output: 675
CleverTapSDK.xcodeproj/project.pbxproj (8)
169-170: New event database files addedThe changes introduce new event database files:
CTEventDatabase.hheader fileCTEventDatabase.mimplementation fileThese files appear to be part of implementing the new user-level event log tracking system mentioned in the PR objectives.
Also applies to: 789-790
171-171: Test file added for event databaseA new test file
CTEventDatabaseTests.mhas been added to validate the event database functionality. This is good practice to ensure the new event tracking system works as expected.Also applies to: 791-791
176-181: Additional header and implementation files addedSeveral new files have been added:
CTEventDatabase.h/m- For event database functionalityCTNotificationAction.h/m- For handling notification actionsCTCustomTemplateInAppData.h/m- For custom template in-app notification dataThe changes maintain proper header visibility by setting appropriate attributes (Public/Private).
805-806: Test helper files addedNew test helper files have been added:
CTLocalDataStore+Tests.hCTLocalDataStore+Tests.mThese files will help with testing the local data store functionality, which is good for maintainability and testability.
1333-1350: Proper organization of event database filesThe event database files are well-organized in their own group/folder:
EventDatabase ├── CTEventDatabase.h ├── CTEventDatabase.m └── CTEventDatabaseTests.mThis maintains good project structure and separation of concerns.
2513-2513: Proper integration of new files in build phasesThe new files are correctly integrated into the build phases:
CTCustomTemplateInAppData.min sourcesCTEventDatabase.min sourcesThis ensures the new functionality will be properly compiled into the framework.
Also applies to: 2768-2768
2572-2572: Test integrationThe event database tests are properly integrated into the test target:
4847D1722CDA17C2008DC327 /* CTEventDatabaseTests.m in Sources */This ensures the tests will be run as part of the test suite.
2627-2628: Test helper integrationThe test helper files are properly integrated:
4E0721512D0974B6000DE9C6 /* CTLocalDataStore+Tests.m in Sources */This provides proper support for testing the local data store functionality.
CleverTapSDK/CleverTapEventDetail.m (1)
6-7: LGTM! Comprehensive debug output.The description method has been properly updated to include the new properties with appropriate format specifiers.
CleverTapSDKTests/CTLocalDataStore+Tests.m (1)
14-17: LGTM! Appropriate use of runtime for testing.The implementation correctly uses the Objective-C runtime to access the private queue for testing purposes.
CleverTapSDK/InApps/CTInAppEvaluationManager.h (5)
13-13: Consider impact of new import dependency.The addition of CTLocalDataStore import creates a new dependency. Ensure this doesn't create circular dependencies.
Run the following script to check for potential circular imports:
✅ Verification successful
No circular dependency concerns found
The import of CTLocalDataStore in CTInAppEvaluationManager follows a clean one-way dependency, where the evaluation manager depends on the data store but not vice versa. This maintains a healthy architectural hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential circular dependencies echo "Checking for circular dependencies..." rg -l "CTLocalDataStore|CTInAppEvaluationManager" | xargs -I {} sh -c 'echo "=== {} ==="; rg "^#import" {}'Length of output: 7797
13-13: LGTM! Required import for new dependency.The import supports the integration of local data store functionality.
34-35: Breaking change in initializer signature.The addition of the localDataStore parameter is a breaking change but is appropriate for version 7.1.0 as it supports the new first-time event filtering feature.
Please ensure all callers of this initializer are updated. Run this script to find potential missed updates:
✅ Verification successful
All callers are properly updated to use the new initializer signature
The verification confirms that all instances of
CTInAppEvaluationManagerinitialization in both production and test code are using the new signature with thelocalDataStoreparameter. No instances of the old signature were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old initializer usage rg "initWithAccountId:.+inAppTriggerManager:" --type objcLength of output: 908
Script:
#!/bin/bash # Broader search for CTInAppEvaluationManager alloc/init patterns rg "CTInAppEvaluationManager.+alloc|initWithAccountId:" --type objc -A 2Length of output: 30109
13-13: LGTM! Import aligns with new dependencies.The import of CTLocalDataStore.h is appropriate for the new functionality.
34-35: Breaking change in initializer - Update documentation and migration guide.The addition of the localDataStore parameter is a breaking change that needs to be documented. This change supports the new first-time event filtering feature but will require updates from SDK users.
Please ensure:
- The CHANGELOG.md is updated to reflect this breaking change
- A migration guide is provided for users upgrading from previous versions
- Documentation is updated to explain the role of localDataStore in event filtering
Run this script to verify the documentation updates:
CleverTapSDK/EventDatabase/CTEventDatabase.h (1)
17-17: Consider thread-safety implications of singleton initialization.The singleton pattern with a dispatch queue manager is a good approach, but ensure thread-safe initialization is implemented in the corresponding .m file.
✅ Verification successful
✅ Singleton implementation is properly thread-safe
The implementation in CTEventDatabase.m correctly uses
dispatch_oncewith a static instance variable, ensuring thread-safe initialization. The dependency injection approach with dispatch queue manager is also well implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper singleton implementation ast-grep --pattern 'static $_ *instance = nil; $$$ dispatch_once($_) { $$$ }'Length of output: 89
Script:
#!/bin/bash # Search for sharedInstance implementations and references rg -A 5 "sharedInstanceWithDispatchQueueManager|CTEventDatabase.*shared" --type objc # Backup search for any singleton-related patterns in CTEventDatabase rg -A 10 "static.*CTEventDatabase.*instance|dispatch_once.*CTEventDatabase" --type objcLength of output: 2201
CleverTapSDK/CTLocalDataStore.h (1)
21-29: LGTM! Well-documented deprecation notices.The deprecation notices clearly indicate the replacement methods and version information.
Package.swift (1)
61-62: LGTM! Correct placement of the new header search path.The EventDatabase header search path is properly added alongside existing paths.
CleverTap-iOS-SDK.podspec (1)
20-20: LGTM! Proper inclusion of EventDatabase files for tvOS.The EventDatabase source files are correctly added to the tvOS target's source files list.
CleverTapSDKTests/InApps/InAppHelper.m (1)
70-73: LGTM! Proper initialization of local data store and integration with evaluation manager.The changes correctly:
- Initialize device info and queue manager
- Set up local data store with required dependencies
- Pass the data store to inAppEvaluationManager
Also applies to: 86-86
CleverTapSDK/InApps/Matchers/CTTriggersMatcher.m (4)
16-18: LGTM! Clean property declaration.The dataStore property is correctly declared as a strong reference.
22-27: LGTM! Proper initializer implementation.The initializer correctly:
- Calls super init
- Assigns the dataStore dependency
- Returns self
55-57: LGTM! Good integration with existing match logic.The first-time check is properly integrated into the match method chain.
134-140: LGTM! Clean implementation of first-time event check.The implementation:
- Early returns if firstTimeOnly is false
- Handles both profile attributes and events
- Delegates to dataStore for the actual check
CleverTapSDKTests/CTLocalDataStoreTests.m (1)
156-172: LGTM! Well-implemented helper method.The waitForInitDataStore helper:
- Uses proper expectation handling
- Has reasonable timeout
- Includes error handling
CleverTapSDK/CTConstants.h (1)
281-284: Verify the event database limits.Please confirm:
- Is the max row limit of ~11.5K (2304 * 5) sufficient for your use case?
- Is cleaning up 2304 rows at once optimal for performance?
Run this script to check typical event counts in existing implementations:
✅ Verification successful
The event database limits are well-designed and appropriate
The current configuration:
- Max limit of ~11.5K rows provides sufficient buffer for event storage
- Cleanup threshold of 2,304 rows (20%) is optimal for performance:
- Large enough to avoid frequent cleanups
- Small enough for efficient batch deletion
- Properly indexed for performance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing row limits or cleanup thresholds rg -i "row.*limit|cleanup.*threshold|max.*rows" --type cpp --type objcLength of output: 1795
Script:
#!/bin/bash # Search for documentation about event database limits rg -i "event.*database|row.*limit|cleanup" "README|CHANGELOG|CONTRIBUTING|docs" --type md # Search for performance tests and benchmarks rg -i "performance|benchmark|stress.*test" --type objc -A 5 -B 5 # Search for comments explaining the rationale rg -i "event.*database|row.*limit|cleanup" --type objc --type cpp -A 2 -B 2 --multilineLength of output: 77591
CleverTapSDKTests/InApps/CTTriggersMatcherTest.m (2)
19-30: Well-structured test setup!The initialization of
dataStoreinsetUpfollows good testing practices with proper dependency injection.
85-85: Consistent integration of dataStore across test methods!The modifications maintain consistency by properly initializing CTTriggersMatcher with dataStore across all test methods.
Also applies to: 108-108, 128-128, 152-152, 245-245, 289-289, 349-349, 424-424, 499-499, 530-530, 576-576, 605-605, 639-639, 708-708, 755-755, 787-787, 816-816, 848-848, 871-871, 963-963, 985-985, 1008-1008, 1031-1031, 1054-1054, 1077-1077, 1098-1098, 1132-1132, 1154-1154, 1189-1189, 1216-1216, 1245-1245, 1273-1273, 1302-1302, 1348-1348, 1376-1376, 1405-1405, 1451-1451, 1504-1504, 1541-1541, 1564-1564, 1588-1588, 1615-1615, 1642-1642, 1674-1674, 1697-1697, 1724-1724, 1747-1747, 1771-1771, 1804-1804, 1827-1827, 1850-1850, 1901-1901, 1939-1939, 1971-1971, 2007-2007
CleverTapSDK/CleverTap.m (5)
1963-1965: LGTM! Good use of async persistence.The event persistence is correctly handled asynchronously on a serial queue to prevent race conditions.
476-478: Verify initialization of user last visit timestamp.The code initializes
_userLastVisitTsfrom event logs, but there's a potential edge case whereeventDetailscould be nil andlastTimewould be accessed.✅ Verification successful
The nil check implementation is correct and safe
The code properly handles the nullable
eventDetailswith a ternary operator, setting_userLastVisitTsto -1 when the event details are not found. This follows the SDK's established patterns for handling missing event data.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential nil checks in other implementations rg -A 5 "getUserEventLog.*lastTime"Length of output: 37
Script:
#!/bin/bash # Search for getUserEventLog method usage rg "getUserEventLog" # Search for lastTime property access patterns rg "lastTime" -A 2Length of output: 8230
198-198: LGTM - Property addition for tracking user's last visit timestampThe property declaration follows proper conventions and uses appropriate type for timestamp storage.
3115-3142: LGTM - Well-structured implementation of user event log methodsThe new methods provide a robust user-level event tracking system with:
- Proper validation of personalization settings
- Consistent error handling
- Clear method signatures and return types
476-478: LGTM - Improved app launch event handlingThe changes properly integrate the new user event logging system with app launch tracking:
- Uses new getUserEventLog API to fetch event details
- Includes proper null checks
- Correctly updates the user's last visit timestamp
CHANGELOG.md (2)
6-14: LGTM! Clear documentation of new features.The changelog clearly documents the new user-level event tracking system and its APIs.
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ses/tag/7.1.0) (January 21, 2024) #### Added - Adds support for triggering InApps based on ...(REPEATED_VERBS)
17-24: LGTM! Well-documented API deprecations.The changelog properly documents deprecated APIs with clear migration paths and rationale.
CleverTapSDKTests/InApps/InAppHelper.h (1)
19-19: LGTM! Good integration of local data store.The addition of CTLocalDataStore supports the new first-time event filtering feature.
Please ensure all test files using InAppHelper are updated to initialize this new required dependency. Run this script to check:
Also applies to: 39-39
✅ Verification successful
Tests are properly handling the new dependency
The verification shows that all test files are using InAppHelper correctly. The helper class encapsulates dependency initialization, including the new dataStore property, making explicit initialization unnecessary in individual test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for InAppHelper usage in test files rg "InAppHelper\s+\*\w+\s*=\s*\[\[InAppHelper\s+alloc\]\s+init\]" CleverTapSDKTests/Length of output: 184
Script:
#!/bin/bash # Search for all InAppHelper references in test files echo "=== Searching for all InAppHelper references ===" rg "InAppHelper" CleverTapSDKTests/ -l echo -e "\n=== Checking CTInAppStoreTest.m for dataStore setup ===" rg "dataStore" CleverTapSDKTests/InApps/CTInAppStoreTest.m -C 2Length of output: 713
Script:
#!/bin/bash # Check test files for InAppHelper usage patterns for file in CTLimitsMatcherTest.m CTInAppStoreTest.m CTInAppFCManagerTest.m CTImpressionManagerTest.m CTInAppEvaluationManagerTest.m CTInAppTriggerManagerTest.m; do echo "=== Checking $file ===" rg "InAppHelper" "CleverTapSDKTests/InApps/$file" -B 2 -A 2 doneLength of output: 5371
CleverTapSDKTests/EventDatabase/CTEventDatabase+Tests.h (1)
16-17: Well-structured test helper with good dependency injection!The test category is well-designed, allowing for controlled testing of the new event tracking system by injecting the dispatch queue manager and clock dependencies.
| NSString *updateSQL = [NSString stringWithFormat:@"PRAGMA user_version = %ld;", (long)version]; | ||
|
|
||
| [self.dispatchQueueManager runSerialAsync:^{ | ||
| sqlite3_stmt *statement; | ||
| if (sqlite3_prepare_v2(self->_eventDatabase, [updateSQL UTF8String], -1, &statement, NULL) == SQLITE_OK) { | ||
| sqlite3_bind_int(statement, 1, (int)version); | ||
|
|
||
| int result = sqlite3_step(statement); | ||
| if (result != SQLITE_OK) { | ||
| CleverTapLogStaticInternal(@"SQL Error: %s", sqlite3_errmsg(self->_eventDatabase)); | ||
| } | ||
|
|
||
| sqlite3_finalize(statement); | ||
| } else { | ||
| CleverTapLogStaticInternal(@"Failed to prepare update statement: %s", sqlite3_errmsg(self->_eventDatabase)); | ||
| } | ||
| }]; |
There was a problem hiding this comment.
Fix improper parameter binding in setDatabaseVersion: method.
The SQL statement in setDatabaseVersion: is constructed using NSString stringWithFormat: to embed the version directly into the SQL command. This approach can lead to SQL injection vulnerabilities and prevents proper parameter binding. Additionally, the call to sqlite3_bind_int has no effect because there are no parameter markers (?) in the SQL statement.
Apply this diff to fix the issue:
- NSString *updateSQL = [NSString stringWithFormat:@"PRAGMA user_version = %ld;", (long)version];
+ const char *updateSQL = "PRAGMA user_version = ?";
...
- if (sqlite3_prepare_v2(self->_eventDatabase, [updateSQL UTF8String], -1, &statement, NULL) == SQLITE_OK) {
+ if (sqlite3_prepare_v2(self->_eventDatabase, updateSQL, -1, &statement, NULL) == SQLITE_OK) {
...
- sqlite3_bind_int(statement, 1, (int)version);
+ sqlite3_bind_int(statement, 1, (int)version);Ensure the SQL statement uses a parameter marker and properly binds the version value using sqlite3_bind_int. This prevents SQL injection vulnerabilities and ensures the pragma command executes correctly.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@nishant-clevertap please take care of this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CleverTapSDK/InApps/CustomTemplates/CTTemplateContext.m (1)
264-273: Add input validation and documentation for the action name resolution.The method implements a good fallback chain but could benefit from defensive programming and documentation.
Consider these improvements:
+/** + * Returns the name for the given notification action. + * @param action The notification action (must not be nil) + * @return The action name using the following fallback chain: + * 1. customTemplateInAppData.templateName + * 2. inAppActionTypeString + * 3. Empty string as fallback + */ - (NSString *)actionName:(CTNotificationAction *)action { + if (!action) { + CleverTapLogStaticDebug(@"%@: Action parameter cannot be nil", [self class]); + return @""; + } NSString *name = action.customTemplateInAppData.templateName; if (!name) { name = [CTInAppUtils inAppActionTypeString:action.type]; } if (!name) { name = @""; } return name; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CleverTapSDK/InApps/CustomTemplates/CTTemplateContext.m(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
CleverTapSDK/InApps/CustomTemplates/CTTemplateContext.m (1)
281-281: LGTM! Good refactoring to use the centralized action name resolution.The change improves code maintainability by using the new
actionName:method instead of directly accessing the template name.
* Implement in-app notification did show callback * Update changelog
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CleverTapSDK/CleverTapInAppNotificationDelegate.h (1)
41-49: Consider enhancing documentation and type safety.A few suggestions to improve the implementation:
- The documentation could be more detailed about the structure of the notification dictionary to help implementers understand what data to expect.
- Consider creating a dedicated model object (e.g.,
CTInAppNotification) instead of usingNSDictionaryfor better type safety and IDE support.Example documentation improvement:
/*! @discussion This is called when an in-app notification is rendered to the user. Use this callback to track notification display events or perform custom actions when notifications appear. @param notification The notification JSON object containing: - id: The unique identifier of the notification - type: The type of notification (e.g., "alert", "banner") - content: The notification content and styling properties - extras: Any custom key-value pairs set in dashboard */CHANGELOG.md (1)
4-4: Update the release date to 2025.Since we're in January 2025, the release date should be updated to "January 21, 2025".
-### [Version 7.1.0](https://github.com/CleverTap/clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) +### [Version 7.1.0](https://github.com/CleverTap/clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2025)🧰 Tools
🪛 LanguageTool
[inconsistency] ~4-~4: A new year has begun. Did you mean “January 21, 2025”?
Context: .../clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) #### Added - Adds support for trigger...(DATE_NEW_YEAR)
🪛 Markdownlint (0.37.0)
4-4: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
CleverTapSDK/InApps/CTInAppDisplayManager.m (1)
517-521: LGTM! Consider adding documentation.The implementation correctly notifies the delegate when an in-app notification is shown. Consider adding documentation to describe the method's purpose and parameters.
+/** + * Notifies the delegate that an in-app notification was shown. + * @param notification The in-app notification that was shown. + */ - (void)notifyNotificationDidShow:(CTInAppNotification *)notification {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)CleverTapSDK/CleverTapInAppNotificationDelegate.h(1 hunks)CleverTapSDK/InApps/CTInAppDisplayManager.m(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[inconsistency] ~4-~4: A new year has begun. Did you mean “January 21, 2025”?
Context: .../clevertap-ios-sdk/releases/tag/7.1.0) (January 21, 2024) #### Added - Adds support for trigger...
(DATE_NEW_YEAR)
[grammar] ~6-~6: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ses/tag/7.1.0) (January 21, 2024) #### Added - Adds support for triggering InApps based on ...
(REPEATED_VERBS)
🪛 Markdownlint (0.37.0)
CHANGELOG.md
4-4: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
CleverTapSDK/CleverTapInAppNotificationDelegate.h (1)
41-49: LGTM! The new delegate method completes the notification lifecycle.The addition of
inAppNotificationDidShow:provides a valuable callback that was missing in the notification flow, allowing delegates to track when notifications are actually rendered to users.CleverTapSDK/InApps/CTInAppDisplayManager.m (3)
517-521: LGTM! Well-implemented delegate notification.The implementation correctly follows the delegate pattern with proper nil checks and provides the notification data in JSON format.
598-598: LGTM! Proper sequencing of operations.The notification to the delegate is correctly placed after recording the internal state.
598-598: LGTM!The delegate notification is called at the appropriate time in the notification lifecycle, after recording the notification state and updating the frequency capping manager.
CHANGELOG.md (1)
6-15: LGTM! Well-documented changes.The changelog clearly documents the new features and API additions, providing good context for the changes we saw in the code.
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...ses/tag/7.1.0) (January 21, 2024) #### Added - Adds support for triggering InApps based on ...(REPEATED_VERBS)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
README.md (1)
78-81: Add "the" before "CleverTap iOS SDK".Minor grammatical improvement needed in the description.
-CleverTap iOS SDK supports user-level event log APIs, refer to [User Event Logging](/docs/UserEventLogging.md) for more details and usage examples. +The CleverTap iOS SDK supports user-level event log APIs, refer to [User Event Logging](/docs/UserEventLogging.md) for more details and usage examples.🧰 Tools
🪛 LanguageTool
[uncategorized] ~79-~79: You might be missing the article “the” here.
Context: ...mples. ## 🕹️ User Event Logging APIs CleverTap iOS SDK supports user-level event log A...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
docs/UserEventLogging.md (2)
1-4: Improve the introduction section.The introduction needs several grammatical improvements and clearer structure.
# User Event Logging APIs -CleverTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logging. Be sure to call enablePersonalization (typically once at app launch) prior to using this method. These API call involves a database query and we recommend to call API from background thread like shown below. +The CleverTap iOS SDK 7.1.0 and above provides the following new APIs for user-level event logging. Be sure to call `enablePersonalization()` (typically once at app launch) before using these APIs. Since these APIs involve database queries, we recommend calling them from a background thread as shown below.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...(LIKE_SHOWN_TO_AS_SHOWN)
1-124: Improve documentation structure and add missing details.
- Heading levels should be consistent (use ### for subsections instead of ####)
- Add information about data persistence and limitations
Consider adding the following sections:
- Data persistence details (how long is the event history retained?)
- Performance implications for large event histories
- Best practices for background thread usage
- Limitations and edge cases
Would you like me to provide a revised structure with these additions?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...(LIKE_SHOWN_TO_AS_SHOWN)
[grammar] ~100-~100: It seems that an article is missing. Did you mean “the total number of” or “a total number of”?
Context: ... print("No events found") } ``` ## Get total number of app launches by user #### Objective-C `...(BUNCH_OF)
[uncategorized] ~113-~113: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...cribing: appLaunchCount))") ``` ## Get user last app visit timestamp #### Objective...(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
🪛 Markdownlint (0.37.0)
5-5: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
26-26: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
56-56: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
69-69: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
101-101: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
114-114: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)docs/UserEventLogging.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~79-~79: You might be missing the article “the” here.
Context: ...mples. ## 🕹️ User Event Logging APIs CleverTap iOS SDK supports user-level event log A...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
docs/UserEventLogging.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...
(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...
(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...
(LIKE_SHOWN_TO_AS_SHOWN)
[grammar] ~100-~100: It seems that an article is missing. Did you mean “the total number of” or “a total number of”?
Context: ... print("No events found") } ``` ## Get total number of app launches by user #### Objective-C `...
(BUNCH_OF)
[uncategorized] ~113-~113: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...cribing: appLaunchCount))") ``` ## Get user last app visit timestamp #### Objective...
(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
🪛 Markdownlint (0.37.0)
docs/UserEventLogging.md
5-5: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
26-26: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
56-56: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
69-69: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
101-101: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
114-114: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
docs/UserEventLogging.md
Outdated
| ## Get user event details | ||
| #### Objective-C | ||
| ```objc | ||
| CleverTapEventDetail *event = [[CleverTap sharedInstance] getUserEventLog:@"Product viewed"]; | ||
| if (event) { | ||
| NSString *eventName = event.eventName; | ||
| NSTimeInterval firstTime = event.firstTime; | ||
| NSTimeInterval lastTime = event.lastTime; | ||
| NSInteger eventCount = event.count; | ||
| NSString *deviceId = event.deviceID;NSLog(@"eventName: %@, firstTime: %f, lastTime: %f, count: %ld, deviceID: %@", eventName, firstTime, lastTime, (long)eventCount, deviceId); | ||
| } else { | ||
| NSLog(@"Event not exists"); | ||
| } | ||
| ``` | ||
|
|
||
| #### Swift | ||
| ```swift | ||
| let event: CleverTapEventDetail? = CleverTap.sharedInstance()?.getUserEventLog("Product viewed") | ||
| if let event = event { | ||
| let eventName: String = event.eventName | ||
| let firstTime: Double = event.firstTime | ||
| let lastTime: Double = event.lastTime | ||
| let count: UInt = event.count | ||
| let deviceID: String = event.deviceID | ||
| print("Event name: \(eventName), first time: \(firstTime), last time: \(lastTime), count: \(count), device id: \(deviceID)") | ||
| } else { | ||
| print("Event not exists") | ||
| } | ||
| ``` | ||
|
|
||
| ## Get count of event occurrences | ||
| #### Objective-C | ||
| ```objc | ||
| int eventCount = [[CleverTap sharedInstance] getUserEventLogCount:@"Product viewed"]; | ||
| NSLog(@"Event count: %ld", (long)eventCount); | ||
| ``` | ||
|
|
||
| #### Swift | ||
| ```swift | ||
| let eventCount = CleverTap.sharedInstance()?.getUserEventLogCount("Product viewed") | ||
| print("Event count: \(String(describing: eventCount))") | ||
| ``` | ||
|
|
||
| ## Get full event history for user | ||
| #### Objective-C | ||
| ```objc | ||
| NSDictionary<NSString *, CleverTapEventDetail *> *allEvents = [[CleverTap sharedInstance] getUserEventLogHistory]; | ||
| for (NSString *key in allEvents) { | ||
| NSString *eventName = allEvents[key].eventName; | ||
| NSTimeInterval firstTime = allEvents[key].firstTime; | ||
| NSTimeInterval lastTime = allEvents[key].lastTime; | ||
| NSInteger eventCount = allEvents[key].count; | ||
| NSString *deviceId = allEvents[key].deviceID; | ||
| NSLog(@"Event name: %@, firstTime: %f, lastTime: %f, count: %ld, deviceID: %@", eventName, firstTime, lastTime, (long)eventCount, deviceId); | ||
| } | ||
| ``` | ||
|
|
||
| #### Swift | ||
| ```swift | ||
| let allEvents = CleverTap.sharedInstance()?.getUserEventLogHistory() | ||
| if let allEvents = allEvents { | ||
| for eventDetails in allEvents.values { | ||
| let eventDetails: CleverTapEventDetail = eventDetails as! CleverTapEventDetail | ||
| let eventName: String = eventDetails.eventName | ||
| let firstTime: Double = eventDetails.firstTime | ||
| let lastTime: Double = eventDetails.lastTime | ||
| let count: UInt = eventDetails.count | ||
| let deviceID: String = eventDetails.deviceID | ||
| print("Event name: \(eventName), first time: \(firstTime), last time: \(lastTime), count: \(count), device id: \(deviceID)") | ||
| } | ||
| } else { | ||
| print("No events found") | ||
| } | ||
| ``` | ||
|
|
||
| ## Get total number of app launches by user | ||
| #### Objective-C | ||
| ```objc | ||
| int appLaunchCount = [[CleverTap sharedInstance] getUserAppLaunchCount]; | ||
| NSLog(@"App launched count: %d", appLaunchCount); | ||
| ``` | ||
|
|
||
| #### Swift | ||
| ```swift | ||
| let appLaunchCount = CleverTap.sharedInstance()?.getUserAppLaunchCount() | ||
| print("App launched count: \(String(describing: appLaunchCount))") | ||
| ``` | ||
|
|
||
| ## Get user last app visit timestamp | ||
| #### Objective-C | ||
| ```objc | ||
| NSTimeInterval lastVisit = [[CleverTap sharedInstance] getUserLastVisitTs]; | ||
| NSLog(@"User last visit timestamp: %f", lastVisit); | ||
| ``` | ||
|
|
||
| #### Swift | ||
| ```swift | ||
| let userLastVisit = CleverTap.sharedInstance()?.getUserLastVisitTs() | ||
| print("User last visit timestamp: \(String(describing: userLastVisit))") | ||
| ``` |
There was a problem hiding this comment.
💡 Codebase verification
Add deprecation notice to the User Event Logging documentation
The documentation should include a deprecation notice at the beginning, listing the old APIs being replaced and their new counterparts. While the deprecation is well documented in the source code and CHANGELOG.md, having this information in the API documentation would improve the developer experience.
Add the following mapping:
eventGetFirstTime,eventGetLastTime,eventGetDetail→getUserEventLogeventGetOccurrences→getUserEventLogCountuserGetPreviousVisitTime→getUserLastVisitTsuserGetTotalVisits→getUserAppLaunchCountuserGetEventHistory→getUserEventLogHistory
🔗 Analysis chain
Verify documentation of deprecated APIs.
The documentation should mention the deprecated APIs that are being replaced by these new user-level event logging APIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation of deprecated APIs
echo "Searching for deprecated API documentation..."
# List of deprecated methods to search for
deprecated_methods=(
"eventGetDetail"
"eventGetOccurrences"
"eventGetFirstTime"
"eventGetLastTime"
"userGetPreviousVisitTime"
"userGetTotalVisits"
"userGetEventHistory"
)
# Search in documentation files
for method in "${deprecated_methods[@]}"; do
echo "Checking documentation for $method..."
rg -l "$method" docs/ || echo "No documentation found for $method"
done
Length of output: 2264
Script:
#!/bin/bash
# Search for deprecated method implementations in source code
echo "Searching for deprecated method implementations..."
# List of deprecated methods to search for
deprecated_methods=(
"eventGetDetail"
"eventGetOccurrences"
"eventGetFirstTime"
"eventGetLastTime"
"userGetPreviousVisitTime"
"userGetTotalVisits"
"userGetEventHistory"
)
# Search in source files
for method in "${deprecated_methods[@]}"; do
echo "Checking implementation of $method..."
ast-grep --pattern "$method" || echo "No implementation found for $method"
done
# Also check for any deprecation annotations
rg -i "deprecated|@available"
Length of output: 18399
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...
(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...
(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...
(LIKE_SHOWN_TO_AS_SHOWN)
[grammar] ~100-~100: It seems that an article is missing. Did you mean “the total number of” or “a total number of”?
Context: ... print("No events found") } ``` ## Get total number of app launches by user #### Objective-C `...
(BUNCH_OF)
[uncategorized] ~113-~113: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...cribing: appLaunchCount))") ``` ## Get user last app visit timestamp #### Objective...
(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
🪛 Markdownlint (0.37.0)
5-5: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
26-26: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
56-56: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
69-69: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
101-101: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
114-114: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/UserEventLogging.md (2)
1-4: 🛠️ Refactor suggestionAdd deprecation notice and improve introduction clarity.
The introduction should include:
- A deprecation notice listing the old APIs being replaced
- Grammar improvements
Apply these changes:
# User Event Logging APIs -CleverTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logging. Be sure to call enablePersonalization (typically once at app launch) prior to using this method. These API call involves a database query and we recommend to call API from background thread like shown below. +CleverTap iOS SDK 7.1.0 and above provides the following new APIs to track user-level event logging. These APIs replace the following deprecated methods: + +- `eventGetDetail`, `eventGetLastTime`, `eventGetFirstTime` → `getUserEventLog` +- `eventGetOccurrences` → `getUserEventLogCount` +- `userGetPreviousVisitTime` → `getUserLastVisitTs` +- `userGetTotalVisits` → `getUserAppLaunchCount` +- `userGetEventHistory` → `getUserEventLogHistory` + +## Prerequisites + +1. Call `enablePersonalization` once at app launch before using these APIs +2. Execute these APIs on a background thread as they involve database queries, as shown in the examples below🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing article found.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...(AI_HYDRA_LEO_MISSING_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...(LIKE_SHOWN_TO_AS_SHOWN)
85-99:⚠️ Potential issueImprove Swift code example for safety and consistency.
The Swift code uses force casting which could lead to runtime crashes.
Apply this diff to improve safety:
let allEvents = CleverTap.sharedInstance()?.getUserEventLogHistory() if let allEvents = allEvents { for eventDetails in allEvents.values { - let eventDetails: CleverTapEventDetail = eventDetails as! CleverTapEventDetail - let eventName: String = eventDetails.eventName - let firstTime: Double = eventDetails.firstTime - let lastTime: Double = eventDetails.lastTime - let count: UInt = eventDetails.count - let deviceID: String = eventDetails.deviceID - print("Event name: \(eventName), first time: \(firstTime), last time: \(lastTime), count: \(count), device id: \(deviceID)") + guard let details = eventDetails as? CleverTapEventDetail else { + print("Invalid event details format") + continue + } + print(""" + Event name: \(details.eventName), + first time: \(details.firstTime), + last time: \(details.lastTime), + count: \(details.count), + device id: \(details.deviceID) + """) } } else { print("No events found") }
🧹 Nitpick comments (2)
docs/UserEventLogging.md (2)
5-125: Fix documentation structure and heading hierarchy.The markdown structure has inconsistent heading levels which affects readability and documentation standards.
Apply these changes to the heading structure:
- Use
##for language sections (Objective-C, Swift)- Use
##for main API sections- Use
###for language subsectionsExample:
-#### Objective-C +## Objective-C🧰 Tools
🪛 LanguageTool
[grammar] ~101-~101: It seems that an article is missing. Did you mean “the total number of” or “a total number of”?
Context: ... print("No events found") } ``` ## Get total number of app launches by user #### Objective-C `...(BUNCH_OF)
🪛 Markdownlint (0.37.0)
5-5: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
26-26: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
57-57: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
70-70: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
102-102: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
115-115: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
37-37: Improve error message clarity.The error messages "Event not exists" are grammatically incorrect and not descriptive enough.
Apply these changes:
- NSLog(@"Event not exists"); + NSLog(@"Event '%@' not found", eventName);- print("Event not exists") + print("Event '\(eventName)' not found")Also applies to: 52-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/UserEventLogging.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/UserEventLogging.md
[uncategorized] ~3-~3: Possible missing article found.
Context: ...verTap iOS SDK 7.1.0 and above provides following new APIs to track user level event logg...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~3-~3: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...lization (typically once at app launch) prior to using this method. These API call invol...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~3-~3: The plural determiner ‘these’ does not agree with the singular noun ‘api’.
Context: ...app launch) prior to using this method. These API call involves a database query and we r...
(THIS_NNS)
[uncategorized] ~3-~3: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...These API call involves a database query and we recommend to call API from backgroun...
(COMMA_COMPOUND_SENTENCE)
[grammar] ~3-~3: Did you mean “as”?
Context: ...mend to call API from background thread like shown below. #### Objective-C ```objc ...
(LIKE_SHOWN_TO_AS_SHOWN)
[grammar] ~101-~101: It seems that an article is missing. Did you mean “the total number of” or “a total number of”?
Context: ... print("No events found") } ``` ## Get total number of app launches by user #### Objective-C `...
(BUNCH_OF)
🪛 Markdownlint (0.37.0)
docs/UserEventLogging.md
5-5: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
26-26: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
57-57: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
70-70: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
102-102: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
115-115: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
CleverTapSDK/EventDatabase/CTEventDatabase.m (3)
567-574: Clarify schema migration process incheckAndUpdateDatabaseVersionIn
checkAndUpdateDatabaseVersion, when thecurrentVersionis less thanCLTAP_DATABASE_VERSION, the code sets the database version to the new version and logs that a schema migration is required. However, no migration steps are actually performed, which might cause confusion.Consider updating the log message to reflect that no migration is currently implemented, or implement the necessary migration steps. Here's a suggested adjustment to the log message:
[self setDatabaseVersion:CLTAP_DATABASE_VERSION]; - CleverTapLogStaticInternal(@"Schema migration required. Current version: %ld, Target version: %ld", (long)currentVersion, (long)CLTAP_DATABASE_VERSION); + CleverTapLogStaticInternal(@"Updated database version from %ld to %ld", (long)currentVersion, (long)CLTAP_DATABASE_VERSION); + // TODO: Implement schema migration steps for future version changes.
76-117: Refactor repetitive SQLite operations to improve maintainabilitySeveral methods, such as
insertEvent:normalizedEventName:deviceID:completion:andupdateEvent:forDeviceID:completion:, contain repetitive code for preparing statements, binding parameters, executing steps, and handling errors. Refactoring this common logic into helper methods would reduce code duplication and enhance maintainability.For example, you could create a generic method to handle statement preparation and execution:
- (BOOL)executeSQL:(const char *)sql withBindings:(void (^)(sqlite3_stmt *statement))bindings { sqlite3_stmt *statement; if (sqlite3_prepare_v2(self->_eventDatabase, sql, -1, &statement, NULL) == SQLITE_OK) { if (bindings) { bindings(statement); } int result = sqlite3_step(statement); sqlite3_finalize(statement); return result == SQLITE_DONE || result == SQLITE_ROW; } else { CleverTapLogStaticInternal(@"SQL prepare error: %s", sqlite3_errmsg(self->_eventDatabase)); return NO; } }Then, simplify your methods by using this helper:
// Example usage in insertEvent:normalizedEventName:deviceID:completion: const char *insertSQL = "INSERT INTO CTUserEventLogs (eventName, normalizedEventName, count, firstTs, lastTs, deviceID) VALUES (?, ?, ?, ?, ?, ?)"; BOOL success = [self executeSQL:insertSQL withBindings:^(sqlite3_stmt *statement) { sqlite3_bind_text(statement, 1, [eventName UTF8String], -1, SQLITE_TRANSIENT); // Bind other parameters... }];
418-428: Optimize index creation by moving it to the table creation phaseCurrently, the index on
lastTsis created within thedeleteLeastRecentlyUsedRows:numberOfRowsToCleanup:completion:method, which means the index creation query runs every time the method is called.Move the index creation to the
createTableWithCompletion:method so that the index is created only once when the table is created:// In createTableWithCompletion: const char *createTableSQL = "..."; if (sqlite3_exec(self->_eventDatabase, createTableSQL, NULL, NULL, &errMsg) == SQLITE_OK) { success = YES; + // Create index after table creation + const char *createIndexSQL = "CREATE INDEX IF NOT EXISTS idx_lastTs ON CTUserEventLogs(lastTs);"; + if (sqlite3_exec(self->_eventDatabase, createIndexSQL, NULL, NULL, &errMsg) != SQLITE_OK) { + CleverTapLogStaticInternal(@"Failed to create index on lastTs: %s", errMsg); + sqlite3_free(errMsg); + } // Set the database version... }This change avoids unnecessary execution of the index creation statement on every cleanup operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CleverTapSDK/CleverTapBuildInfo.h(1 hunks)CleverTapSDK/EventDatabase/CTEventDatabase.m(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CleverTapSDK/CleverTapBuildInfo.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| NSString *updateSQL = [NSString stringWithFormat:@"PRAGMA user_version = %ld;", (long)version]; | ||
|
|
||
| [self.dispatchQueueManager runSerialAsync:^{ | ||
| sqlite3_stmt *statement; | ||
| if (sqlite3_prepare_v2(self->_eventDatabase, [updateSQL UTF8String], -1, &statement, NULL) == SQLITE_OK) { | ||
| sqlite3_bind_int(statement, 1, (int)version); | ||
|
|
||
| int result = sqlite3_step(statement); | ||
| if (result != SQLITE_OK) { | ||
| CleverTapLogStaticInternal(@"SQL Error: %s", sqlite3_errmsg(self->_eventDatabase)); | ||
| } | ||
|
|
||
| sqlite3_finalize(statement); | ||
| } else { | ||
| CleverTapLogStaticInternal(@"Failed to prepare update statement: %s", sqlite3_errmsg(self->_eventDatabase)); | ||
| } | ||
| }]; |
There was a problem hiding this comment.
@nishant-clevertap please take care of this
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (1)
209-213:⚠️ Potential issueAvoid performing asynchronous operations in
deallocIn the
deallocmethod, calling[self.dispatchQueueManager runSerialAsync:]may cause unexpected behavior because the block may not execute before the object is fully deallocated. This can result in the database not being properly closed.To ensure proper cleanup, perform the database closing operation synchronously.
Apply this diff to fix the issue:
- (void)dealloc { - [self.dispatchQueueManager runSerialAsync:^{ + (void)dealloc { + [self.dispatchQueueManager runSerialSync:^{ [self closeDatabase]; }]; }
🧹 Nitpick comments (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (1)
577-581: Use Application Support directory for database storageStoring the database file in the Documents directory may expose it to the user if file sharing is enabled. It's recommended to store app-internal data in the Application Support directory to prevent unintended access.
Apply this diff to change the database path:
- (NSString *)databasePath { - NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES); + NSArray *paths = NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory, NSUserDomainMask, YES); NSString *directory = [paths objectAtIndex:0]; + // Ensure the directory exists + if (![[NSFileManager defaultManager] fileExistsAtPath:directory]) { + [[NSFileManager defaultManager] createDirectoryAtPath:directory withIntermediateDirectories:YES attributes:nil error:nil]; + } - return [documentsDirectory stringByAppendingPathComponent:@"CleverTap-Events.db"]; + return [directory stringByAppendingPathComponent:@"CleverTap-Events.db"]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m(1 hunks)
🧰 Additional context used
📓 Learnings (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (4)
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class currently uses an async operation for database cleanup, which should be changed to a sync operation to ensure proper cleanup during deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class needs special handling for database cleanup operations to ensure proper closure before deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK uses dispatch semaphores and synchronous execution during initialization to ensure thread safety in database operations.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK ensures thread safety through semaphores, direct execution on serial queue, timeout handling, and thread-safe singleton pattern using dispatch_once.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (1)
209-213:⚠️ Potential issueAvoid performing asynchronous operations in
deallocThe
deallocmethod is calling[self.dispatchQueueManager runSerialAsync:], which may cause unexpected behavior because the object may be deallocated before the async block is executed. This can lead to resource leaks or crashes.To ensure proper cleanup, perform the database closing operation synchronously.
Apply this diff to fix the issue:
- [self.dispatchQueueManager runSerialAsync:^{ + [self.dispatchQueueManager runSerialSync:^{ [self closeDatabase]; }];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m(1 hunks)
🧰 Additional context used
📓 Learnings (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (4)
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class currently uses an async operation for database cleanup, which should be changed to a sync operation to ensure proper cleanup during deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class needs special handling for database cleanup operations to ensure proper closure before deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK uses dispatch semaphores and synchronous execution during initialization to ensure thread safety in database operations.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK ensures thread safety through semaphores, direct execution on serial queue, timeout handling, and thread-safe singleton pattern using dispatch_once.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
CleverTapSDK/EventDatabase/CTEventDatabase.m (2)
209-213:⚠️ Potential issueAvoid performing asynchronous operations in
deallocPerforming asynchronous operations in the
deallocmethod may lead to unexpected behavior because the object might be deallocated before the asynchronous block executes. It's recommended to perform the cleanup synchronously to ensure the database is properly closed during deallocation.Apply this diff to fix the issue:
- (void)dealloc { - [self.dispatchQueueManager runSerialAsync:^{ + [self.dispatchQueueManager runSerialSync:^{ [self closeDatabase]; }]; }
551-567:⚠️ Potential issueFix improper parameter binding in
setDatabaseVersion:SQLite does not support parameter binding in PRAGMA statements. Using
sqlite3_bind_intwithPRAGMA user_version = ?will not set the version correctly.Apply this diff to fix the issue:
- const char *updateSQL = "PRAGMA user_version = ?"; [self.dispatchQueueManager runSerialAsync:^{ - sqlite3_stmt *statement; - if (sqlite3_prepare_v2(self->_eventDatabase, updateSQL, -1, &statement, NULL) == SQLITE_OK) { - sqlite3_bind_int(statement, 1, (int)version); - - int result = sqlite3_step(statement); - if (result != SQLITE_DONE) { - CleverTapLogStaticInternal(@"SQL Error: %s", sqlite3_errmsg(self->_eventDatabase)); - } - - sqlite3_finalize(statement); - } else { - CleverTapLogStaticInternal(@"Failed to prepare update statement: %s", sqlite3_errmsg(self->_eventDatabase)); - } + char *errMsg = NULL; + NSString *updateSQL = [NSString stringWithFormat:@"PRAGMA user_version = %ld;", (long)version]; + int result = sqlite3_exec(self->_eventDatabase, [updateSQL UTF8String], NULL, NULL, &errMsg); + if (result != SQLITE_OK) { + CleverTapLogStaticInternal(@"SQL Error setting database version: %s", errMsg); + sqlite3_free(errMsg); + } }];This change constructs the SQL statement with the
versionvalue embedded directly, which is acceptable here sinceversionis an internally controlled integer. This ensures the database version is set correctly without relying on parameter binding, which isn't supported in PRAGMA statements.
🧹 Nitpick comments (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (1)
403-478: Avoid unnecessary transaction rollback when no rows are deletedIn
deleteLeastRecentlyUsedRows:numberOfRowsToCleanup:completion:, if no rows need to be deleted, the transaction is started but not committed, leading to an unnecessary rollback. This can impact performance.Apply this diff to commit the transaction when no deletions occur:
// Begin a transaction to ensure atomicity sqlite3_exec(self->_eventDatabase, "BEGIN TRANSACTION;", NULL, NULL, NULL); // ... existing code ... } else { success = YES; + // Commit the transaction even if no rows are deleted + sqlite3_exec(self->_eventDatabase, "COMMIT;", NULL, NULL, NULL); } // Commit or rollback the transaction based on success if (success) { - sqlite3_exec(self->_eventDatabase, "COMMIT;", NULL, NULL, NULL); } else { sqlite3_exec(self->_eventDatabase, "ROLLBACK;", NULL, NULL, NULL); }By committing the transaction when no deletions are necessary, you avoid the overhead of a rollback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m(1 hunks)
🧰 Additional context used
📓 Learnings (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (4)
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class currently uses an async operation for database cleanup, which should be changed to a sync operation to ensure proper cleanup during deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:210-213
Timestamp: 2025-01-23T07:03:19.840Z
Learning: The dealloc method in CTEventDatabase class needs special handling for database cleanup operations to ensure proper closure before deallocation.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK uses dispatch semaphores and synchronous execution during initialization to ensure thread safety in database operations.
Learnt from: darshanclevertap
PR: CleverTap/clevertap-ios-sdk#399
File: CleverTapSDK/EventDatabase/CTEventDatabase.m:480-490
Timestamp: 2025-01-23T07:03:18.609Z
Learning: The CTEventDatabase in CleverTap iOS SDK ensures thread safety through semaphores, direct execution on serial queue, timeout handling, and thread-safe singleton pattern using dispatch_once.
🔇 Additional comments (1)
CleverTapSDK/EventDatabase/CTEventDatabase.m (1)
377-395:⚠️ Potential issueEnsure
errMsgis freed in all execution pathsIn the
deleteAllRowsWithCompletion:method, ifsqlite3_execfails,errMsgis allocated but not freed in all execution paths, which can lead to a memory leak. Ensure thatsqlite3_free(errMsg);is called whenevererrMsgis notNULL.Apply this diff to fix the issue:
if (result == SQLITE_OK) { success = YES; + if (errMsg != NULL) { + sqlite3_free(errMsg); + } } else { CleverTapLogStaticInternal(@"SQL Error deleting all rows from CTUserEventLogs: %s", errMsg); sqlite3_free(errMsg); }This ensures that
errMsgis freed regardless of the outcome ofsqlite3_exec.Likely invalid or redundant comment.
Added
getUserEventLog(:): Get details about a specific eventgetUserEventLogCount(:): Get count of times an event occurredgetUserLastVisitTs(): Get timestamp of user's last app visitgetUserAppLaunchCount(): Get total number of times user has launched the appgetUserEventLogHistory(): Get full event history for current userAPI Changes
eventGetDetail(:): UsegetUserEventLog()instead for user-specific event detailseventGetOccurrences(:): UsegetUserEventLogCount()instead for user-specific event countseventGetFirstTime(:): UsegetUserEventLog()instead for user-specific first occurrence timestampeventGetLastTime(:): UsegetUserEventLog()instead for user-specific last occurrence timestampuserGetPreviousVisitTime(): UsegetUserLastVisitTs()instead for user-specific last visit timestampuserGetTotalVisits(): UsegetUserAppLaunchCount()instead for user-specific app launch countuserGetEventHistory(): UsegetUserEventLogHistory()instead for user-specific event historySummary by CodeRabbit
Release Notes for CleverTap iOS SDK v7.1.0
New Features
Improvements
InAppHelperfor better data handling.Deprecations
Performance