[iOS]: Added buffering and flushing logic for event emitters #312
Conversation
Update master with release 3.0.0
Release v3.1.0
Release 3.2.0
Release v3.3.0
Release v3.3.1
|
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 update introduces an event buffering mechanism for CleverTap events in the iOS plugin, with new Objective-C classes and methods to support event queuing and emission. It standardizes event names in the Dart interface, removes platform checks, and updates notification dispatching logic to use the new buffering approach. Xcode project files are updated for internal consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Dart
participant FlutterPlugin
participant CleverTapPlugin
participant EventBuffer
Dart->>FlutterPlugin: setHandler(eventName)
FlutterPlugin->>CleverTapPlugin: invokeMethod("startEmission", eventName)
CleverTapPlugin->>CleverTapPlugin: startObservingEvent(eventName)
CleverTapPlugin->>EventBuffer: Flush buffered events for eventName
loop On CleverTap event
CleverTapPlugin->>CleverTapPlugin: sendEventOnObserving(eventName, body)
alt Buffering enabled and not observed
CleverTapPlugin->>EventBuffer: Buffer event
else Observed
CleverTapPlugin->>FlutterPlugin: send event to Dart
end
end
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
kushCT
requested review from
akashvercetti,
nzagorchev,
nishant-clevertap and
vasct
|
@coderabbitai 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: 4
🔭 Outside diff range comments (1)
lib/clevertap_plugin.dart (1)
247-248:⚠️ Potential issueFix event name inconsistencies.
The event names passed to
invokeStartEmissiondon't match the corresponding handler method names:
- Line 247: Uses
'CleverTapInAppNotificationShowed'but handler expects"inAppNotificationShow"(line 93)- Line 325: Uses
'CleverTapPushAmpPayloadReceived'but handler expects"pushAmpPayloadReceived"(line 143)void setCleverTapInAppNotificationShowHandler(CleverTapInAppNotificationShowHandler handler) { - invokeStartEmission('CleverTapInAppNotificationShowed'); + invokeStartEmission('inAppNotificationShow'); cleverTapInAppNotificationShowHandler = handler; }void setCleverTapPushAmpPayloadReceivedHandler(CleverTapPushAmpPayloadReceivedHandler handler) { - invokeStartEmission('CleverTapPushAmpPayloadReceived'); + invokeStartEmission('pushAmpPayloadReceived'); cleverTapPushAmpPayloadReceivedHandler = handler; }Also applies to: 325-326
🧹 Nitpick comments (2)
ios/Classes/CleverTapPlugin.m (1)
1793-1806: Enhance the event flushing implementation.The flushing logic is correct but could be more robust:
- Add error handling for method channel invocations:
for (CleverTapPluginPendingEvent *event in events) { - [self.nativeToDartMethodChannel invokeMethod:event.name arguments:event.body]; + @try { + [self.nativeToDartMethodChannel invokeMethod:event.name arguments:event.body]; + } @catch (NSException *exception) { + NSLog(@"CleverTapFlutter: Failed to flush event %@: %@", event.name, exception); + } }
- Consider memory management for the observedEvents set - it grows indefinitely:
+// Optional: Add method to clear observed events when listener is removed +- (void)stopObservingEvent:(NSString *)eventName { + [observedEvents removeObject:eventName]; +}lib/clevertap_plugin.dart (1)
8-10: Remove duplicate import.The
package:flutter/foundation.dartimport is duplicated on lines 8 and 10.import 'package:flutter/services.dart'; -import 'package:flutter/foundation.dart' show kIsWeb; +import 'package:flutter/foundation.dart' show kIsWeb;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
example/ios/Runner.xcodeproj/project.pbxproj(9 hunks)ios/Classes/CleverTapPlugin.h(1 hunks)ios/Classes/CleverTapPlugin.m(8 hunks)ios/Classes/CleverTapPluginAppFunctionPresenter.m(1 hunks)ios/Classes/CleverTapPluginPendingEvent.h(1 hunks)ios/Classes/CleverTapPluginPendingEvent.m(1 hunks)ios/Classes/CleverTapPluginTemplatePresenter.m(1 hunks)lib/clevertap_plugin.dart(3 hunks)
🔇 Additional comments (11)
ios/Classes/CleverTapPlugin.h (1)
36-36: LGTM: Clean method declaration for event buffering.The new method declaration follows proper Objective-C conventions and clearly indicates its purpose for conditional event emission based on observation state.
ios/Classes/CleverTapPluginAppFunctionPresenter.m (1)
16-16: LGTM: Proper integration with new buffering mechanism.The switch from
postNotificationWithName:andBody:tosendEventOnObserving:andBody:correctly integrates with the new event buffering system while maintaining the same event data structure.ios/Classes/CleverTapPluginTemplatePresenter.m (1)
16-16: LGTM: Consistent adoption of event buffering mechanism.Both method calls correctly switch to the new
sendEventOnObserving:andBody:approach, maintaining consistency with the event buffering system while preserving the existing event data structure and logic.Also applies to: 22-22
ios/Classes/CleverTapPluginPendingEvent.h (1)
1-14: LGTM: Clean and well-structured header file.The
CleverTapPluginPendingEventclass is appropriately designed as a simple data container for buffered events. The memory management annotations and non-null specifications are correctly applied.example/ios/Runner.xcodeproj/project.pbxproj (1)
12-12: LGTM: Routine Xcode project maintenance.The UUID updates for CocoaPods references and build phase reordering are standard Xcode project maintenance changes with no impact on functionality.
Also applies to: 37-37, 40-40, 45-45, 57-57, 66-66, 76-78, 88-88, 156-156, 163-163, 222-242, 260-260
ios/Classes/CleverTapPlugin.m (4)
19-19: LGTM: Correct import for the new pending event class.
67-91: LGTM: Proper initialization of buffering collections.The static collection initialization using implicit dispatch_once through the singleton pattern is appropriate, and the bufferable events set is correctly configured.
127-131: LGTM: Clean implementation of startEmission method handling.The method correctly extracts the event name and delegates to the appropriate observation method.
1685-1765: LGTM: Consistent conversion of delegate methods to use buffering.All delegate methods correctly use the new
sendEventOnObserving:andBody:method instead of posting notifications directly. The pattern is consistent across all event types.lib/clevertap_plugin.dart (2)
227-337: LGTM! Event name standardization looks good.The standardization of event names to match the handler method names is correctly implemented across most of the modified setter methods. This should improve consistency between the Dart side and native implementations.
222-224:✅ Verification successful
Verify cross-platform compatibility after removing platform checks.
The
invokeStartEmissionmethod now makes unconditional native method calls. Ensure this doesn't cause issues on platforms that don't support thestartEmissionmethod.Based on the AI summary, Android-only platform checks were removed. Let me verify if this change might cause issues on other platforms:
🏁 Script executed:
#!/bin/bash # Description: Check if startEmission method is implemented across all platforms # Search for startEmission implementation in iOS and Android native code echo "Searching for startEmission implementations..." fd "CleverTapPlugin" --type f --extension m --extension java --extension kt | xargs rg -A 5 "startEmission" # Check for platform-specific method channel implementations rg -A 10 -B 5 "methodCallHandler.*startEmission\|startEmission.*methodCallHandler"Length of output: 1188
🏁 Script executed:
#!/bin/bash # List all Java and Kotlin source files under the Android plugin directory (if present) echo "Java/Kotlin files in android/src/main:" find android/src/main -type f \( -name "*.java" -o -name "*.kt" \) # Search within those files for any handling of 'startEmission' echo "Searching for 'startEmission' in Android plugin sources..." rg -n "startEmission" -C5 android/src/mainLength of output: 4243
startEmission is implemented on both iOS and Android
I’ve confirmed that the
startEmissionmethod is handled in:
- iOS: ios/Classes/CleverTapPlugin.m
- Android: android/src/main/java/com/clevertap/clevertap_plugin/DartToNativePlatformCommunicator.kt
Since both mobile platforms support
startEmission, the unconditional call is safe.
| - (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body { | ||
| self = [super init]; | ||
| if (self) { | ||
| _name = name; | ||
| _body = body; | ||
| } | ||
| return self; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and consider defensive copying.
The initializer should validate input parameters and consider copying the body dictionary to prevent external modifications.
- (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body {
self = [super init];
if (self) {
- _name = name;
- _body = body;
+ if (!name) {
+ return nil;
+ }
+ _name = [name copy];
+ _body = body ? [body copy] : @{};
}
return self;
}This ensures:
- Non-nil name parameter (essential for event identification)
- Defensive copying to prevent external mutations
- Safe handling of nil body parameter
🤖 Prompt for AI Agents
In ios/Classes/CleverTapPluginPendingEvent.m around lines 5 to 12, the
initializer lacks input validation and defensive copying. Modify the
initWithName:body: method to check that the name parameter is non-nil and handle
the case where body might be nil. Also, perform a defensive copy of the body
dictionary to prevent external modifications after initialization.
| // Create the reset buffer block | ||
| resetBufferBlock = dispatch_block_create(0, ^{ | ||
| [pendingEvents removeAllObjects]; | ||
| isBufferingEnabled = NO; | ||
| }); | ||
|
|
||
| // Schedule buffer cleanup after app finishes launching | ||
| dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(kEventBufferTimeoutSeconds * NSEC_PER_SEC)), | ||
| dispatch_get_main_queue(), | ||
| resetBufferBlock); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the timeout logic for buffer management.
The current timeout implementation completely disables buffering and clears all pending events after 5 seconds. This approach is too aggressive and could cause legitimate events to be lost.
Consider a more nuanced approach:
-resetBufferBlock = dispatch_block_create(0, ^{
- [pendingEvents removeAllObjects];
- isBufferingEnabled = NO;
-});
+resetBufferBlock = dispatch_block_create(0, ^{
+ // Only clear events older than timeout, keep buffering enabled
+ NSLog(@"CleverTapFlutter: Buffer timeout reached, clearing old events");
+ // Consider keeping buffering enabled for future events
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ios/Classes/CleverTapPlugin.m around lines 102 to 111, the current logic
clears all pending events and disables buffering after a fixed 5-second timeout,
which is too aggressive and risks losing legitimate events. Modify the timeout
handling to implement a more nuanced approach, such as extending the timeout
duration, conditionally clearing events based on specific criteria, or
implementing a retry mechanism to preserve important events instead of clearing
them outright after the timeout.
| - (void)sendEventOnObserving:(NSString *)name andBody:(NSDictionary *)body { | ||
| // Only buffer events that are explicitly marked as bufferable | ||
| if ([bufferableEvents containsObject:name] && | ||
| ![observedEvents containsObject:name] && | ||
| isBufferingEnabled) { | ||
|
|
||
| if (!pendingEvents[name]) { | ||
| pendingEvents[name] = [NSMutableArray array]; | ||
| } | ||
|
|
||
| // Create and add the event to the buffer without limit | ||
| CleverTapPluginPendingEvent *event = [[CleverTapPluginPendingEvent alloc] initWithName:name body:body]; | ||
| [pendingEvents[name] addObject:event]; | ||
| NSLog(@"CleverTapFlutter: Buffering event %@ for later emission", name); | ||
| return; | ||
| } | ||
|
|
||
| // Otherwise post the notification normally | ||
| [[NSNotificationCenter defaultCenter] postNotificationName:name object:nil userInfo:body]; | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the event buffering implementation.
The buffering logic is sound but needs enhancements for production use:
- Add bounds checking to prevent memory issues:
+static const NSUInteger kMaxEventsPerType = 100;
if (!pendingEvents[name]) {
pendingEvents[name] = [NSMutableArray array];
}
+// Prevent unbounded growth
+if ([pendingEvents[name] count] >= kMaxEventsPerType) {
+ [pendingEvents[name] removeObjectAtIndex:0]; // Remove oldest
+}-
Add thread safety (as mentioned in previous comment)
-
Improve logging with more context:
-NSLog(@"CleverTapFlutter: Buffering event %@ for later emission", name);
+NSLog(@"CleverTapFlutter: Buffering event %@ (total: %lu) for later emission",
+ name, (unsigned long)[pendingEvents[name] count]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - (void)sendEventOnObserving:(NSString *)name andBody:(NSDictionary *)body { | |
| // Only buffer events that are explicitly marked as bufferable | |
| if ([bufferableEvents containsObject:name] && | |
| ![observedEvents containsObject:name] && | |
| isBufferingEnabled) { | |
| if (!pendingEvents[name]) { | |
| pendingEvents[name] = [NSMutableArray array]; | |
| } | |
| // Create and add the event to the buffer without limit | |
| CleverTapPluginPendingEvent *event = [[CleverTapPluginPendingEvent alloc] initWithName:name body:body]; | |
| [pendingEvents[name] addObject:event]; | |
| NSLog(@"CleverTapFlutter: Buffering event %@ for later emission", name); | |
| return; | |
| } | |
| // Otherwise post the notification normally | |
| [[NSNotificationCenter defaultCenter] postNotificationName:name object:nil userInfo:body]; | |
| } | |
| - (void)sendEventOnObserving:(NSString *)name andBody:(NSDictionary *)body { | |
| // Only buffer events that are explicitly marked as bufferable | |
| if ([bufferableEvents containsObject:name] && | |
| ![observedEvents containsObject:name] && | |
| isBufferingEnabled) { | |
| // Cap the number of buffered events to avoid memory issues | |
| static const NSUInteger kMaxEventsPerType = 100; | |
| if (!pendingEvents[name]) { | |
| pendingEvents[name] = [NSMutableArray array]; | |
| } | |
| // Prevent unbounded growth | |
| if ([pendingEvents[name] count] >= kMaxEventsPerType) { | |
| [pendingEvents[name] removeObjectAtIndex:0]; // evict oldest | |
| } | |
| // Create and add the event to the buffer | |
| CleverTapPluginPendingEvent *event = | |
| [[CleverTapPluginPendingEvent alloc] initWithName:name body:body]; | |
| [pendingEvents[name] addObject:event]; | |
| NSLog(@"CleverTapFlutter: Buffering event %@ (total: %lu) for later emission", | |
| name, (unsigned long)[pendingEvents[name] count]); | |
| return; | |
| } | |
| // Otherwise post the notification normally | |
| [[NSNotificationCenter defaultCenter] postNotificationName:name | |
| object:nil | |
| userInfo:body]; | |
| } |
🤖 Prompt for AI Agents
In ios/Classes/CleverTapPlugin.m around lines 1771 to 1791, enhance the
sendEventOnObserving method by adding bounds checking to limit the size of the
pendingEvents buffer to prevent memory issues, implement thread safety by
synchronizing access to shared resources like pendingEvents to avoid race
conditions, and improve logging by including more context such as the current
buffer size and event details when buffering events.
| static NSMutableDictionary<NSString *, NSMutableArray<CleverTapPluginPendingEvent *> *> *pendingEvents; | ||
| static NSMutableSet<NSString *> *bufferableEvents; | ||
| static NSMutableSet<NSString *> *observedEvents; | ||
| static BOOL isBufferingEnabled = YES; | ||
| static const NSTimeInterval kEventBufferTimeoutSeconds = 5.0; // 5 seconds timeout for pending event to get removed | ||
| static dispatch_block_t resetBufferBlock; | ||
|
|
There was a problem hiding this comment.
Address thread safety concerns with static collections.
The static collections (pendingEvents, bufferableEvents, observedEvents) are accessed from multiple threads without synchronization, which can lead to race conditions and crashes.
Consider adding synchronization using either:
@synchronizedblocks around collection access- A dedicated serial dispatch queue for buffering operations
- Using thread-safe collection classes
+static dispatch_queue_t bufferingQueue;
// In init method:
+bufferingQueue = dispatch_queue_create("com.clevertap.buffering", DISPATCH_QUEUE_SERIAL);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static NSMutableDictionary<NSString *, NSMutableArray<CleverTapPluginPendingEvent *> *> *pendingEvents; | |
| static NSMutableSet<NSString *> *bufferableEvents; | |
| static NSMutableSet<NSString *> *observedEvents; | |
| static BOOL isBufferingEnabled = YES; | |
| static const NSTimeInterval kEventBufferTimeoutSeconds = 5.0; // 5 seconds timeout for pending event to get removed | |
| static dispatch_block_t resetBufferBlock; | |
| static NSMutableDictionary<NSString *, NSMutableArray<CleverTapPluginPendingEvent *> *> *pendingEvents; | |
| static NSMutableSet<NSString *> *bufferableEvents; | |
| static NSMutableSet<NSString *> *observedEvents; | |
| static BOOL isBufferingEnabled = YES; | |
| static const NSTimeInterval kEventBufferTimeoutSeconds = 5.0; // 5 seconds timeout for pending event to get removed | |
| static dispatch_block_t resetBufferBlock; | |
| static dispatch_queue_t bufferingQueue; |
🤖 Prompt for AI Agents
In ios/Classes/CleverTapPlugin.m around lines 30 to 36, the static collections
pendingEvents, bufferableEvents, and observedEvents are accessed from multiple
threads without synchronization, risking race conditions. To fix this, introduce
a dedicated serial dispatch queue to synchronize all accesses and mutations to
these collections, ensuring thread safety. Replace direct accesses with
dispatch_async or dispatch_sync calls on this queue to serialize operations.
Background
If an event is posted before
CleverTapPluginstarts observing, the event will be lost.For example, if an event is sent on
AppDelegate didFinishLaunchingWithOptionsit will not be received in the Dart listener.This currently happens with the
CleverTapProfileDidInitializewhich is also sent/posted before the events are observed.Implementation
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores