Auto Flush Events on Network Restore#966
Open
deeksha-rgb wants to merge 58 commits intotask/release/8.1.0from
Open
Auto Flush Events on Network Restore#966deeksha-rgb wants to merge 58 commits intotask/release/8.1.0from
deeksha-rgb wants to merge 58 commits intotask/release/8.1.0from
Conversation
…ork APIs - Refine NetworkMonitor: use applicationContext, add @volatile on _currentState, add NET_CAPABILITY_VALIDATED for captive portal detection, add getNetworkTypeString() - Update DeviceInfo to accept NetworkMonitor in constructor and delegate isWifiConnected() - Update CleverTapFactory to create NetworkMonitor before DeviceInfo and pass to CoreState - Update EventQueueManager to use networkMonitor instance instead of deprecated static calls - Fix InAppController to use NetworkMonitor.isNetworkOnline() replacing removed static method - Update MockDeviceInfo and CallbackManagerTest constructors for new DeviceInfo signature - Add NetworkMonitorTest
· Removed companion object (static isNetworkOnline) — memory leak risk,wasteful (new ConnectivityManager every call), ignores instance state · Replaced with private checkCurrentConnectivity() instance method · Removed _currentState, single source of truth: _stateFlow.value - BitmapDownloadRequestHandler.kt: · Removed NetworkManager/NetworkMonitor static dependency · Direct ConnectivityManager check (open class cannot use internal NetworkMonitor instance)
- Replace synchronous system queries inside NetworkCallback with direct callback arguments to avoid race conditions - onAvailable: mark isAvailable=true, wait for onCapabilitiesChanged - onLost/onUnavailable: directly set NetworkState.DISCONNECTED - onCapabilitiesChanged: calculate state from passed NetworkCapabilities - Update getNetworkTypeFromCapabilities() to accept capabilities param
- Remove legacy activeNetworkInfo fallback (pre API 23) - Remove Build.VERSION.SDK_INT checks
- Added ConnectivityManager?.isNetworkAvailable() extension function in NetworkExtensions.kt (network package) with NET_CAPABILITY_INTERNET and NET_CAPABILITY_VALIDATED checks - Updated NetworkMonitor.checkCurrentConnectivity() to use the shared extension instead of inline duplication - Updated BitmapDownloadRequestHandler to use the shared extension, removing redundant connectivity logic
…ght after config init so network monitoring starts earlier during SDK initialization
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.kt`:
- Line 90: The network callback path (registerNetworkCallback) is a no-op on API
< 24 leaving _stateFlow static and observeNetworkRestore() never firing; add a
fallback for older APIs by detecting SDK < 24 inside the registerNetworkCallback
method and either (a) register a ConnectivityManager broadcast receiver for
CONNECTIVITY_ACTION or (b) start a small periodic poll using
ConnectivityManager.getActiveNetworkInfo()/getNetworkCapabilities() to detect
connectivity changes, update the same _stateFlow variable when connectivity
changes, and invoke the existing onNetworkRestored pathway so
observeNetworkRestore() works; ensure the fallback is cleaned up in the same
teardown/unregister path and reference the existing symbols
registerNetworkCallback, _stateFlow, observeNetworkRestore, and
onNetworkRestored when implementing the alternative.
- Around line 88-90: observeNetworkRestore is currently started after
registerNetworkCallback which can cause the collector's drop(1) to discard a
genuine restore event and the launched coroutine isn't tracked or canceled in
cleanup(), causing leaks; fix by initializing the observer before registering
the platform callback (call observeNetworkRestore() before
registerNetworkCallback()), change the collector launch inside
observeNetworkRestore to store its Job in a field (e.g., networkRestoreJob) and
cancel that Job in cleanup(), and ensure the collector logic on _stateFlow (and
any use of drop(1)) still correctly ignores only the initial emission and not a
real transition to CONNECTED by adjusting subscription order as above.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2143f362-6d29-48cb-ba02-693b15361ba7
📒 Files selected for processing (8)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/events/EventQueueManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/events/EventQueueManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.ktclevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkMonitorTest.kt
💤 Files with no reviewable changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/events/EventQueueManager.java
Anush-Shand
changed the title
Contributor
Author
|
@coderabbitai Addressed. Please check |
…to-flush-on-network-restore
…restore -resolved code rabbit comment
Anush-Shand
reviewed
Mar 31, 2026
| } | ||
|
|
||
|
|
||
| // only call async |
Contributor
There was a problem hiding this comment.
Instead of comment, can use an annotation like @workerthread
Collaborator
There was a problem hiding this comment.
should be a suspend func in that case?
Contributor
Author
There was a problem hiding this comment.
The function is already called inside postAsyncSafelyTask which runs it on a background thread
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
78-88: Note: callback executes on IO dispatcher; multiple calls register multiple observers.The delegation and observation pattern look correct. Two observations:
The
onRestoredcallback executes onDispatchers.IOcontext. Current callers (EventQueueManager.flushQueueAsync) handle this appropriately by posting to their own executor.Each call to
observeNetworkRestorelaunches a new collector without a mechanism to cancel previous ones. If this method is called multiple times (e.g., due to SDK reinitialization edge cases), duplicate callbacks would fire. Consider documenting that this should only be called once perNetworkManagerinstance, or returning aJobfor cancellation if needed in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt` around lines 78 - 88, The observeNetworkRestore currently launches a collector on networkManagerScope each call and runs on Dispatchers.IO, so repeated calls create duplicate observers and callbacks. Modify observeNetworkRestore (and its callers) to return the launched Job (i.e., return the result of networkManagerScope.launch) so callers can cancel the observer when reinitializing; additionally document in the KDoc for observeNetworkRestore and isNetworkOnline that the onRestored callback is invoked on the IO dispatcher and callers should marshal to their own executors if needed. Ensure you reference observeNetworkRestore, networkManagerScope, networkMonitor.networkRestoreEvents, and onRestored when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt`:
- Around line 78-88: The observeNetworkRestore currently launches a collector on
networkManagerScope each call and runs on Dispatchers.IO, so repeated calls
create duplicate observers and callbacks. Modify observeNetworkRestore (and its
callers) to return the launched Job (i.e., return the result of
networkManagerScope.launch) so callers can cancel the observer when
reinitializing; additionally document in the KDoc for observeNetworkRestore and
isNetworkOnline that the onRestored callback is invoked on the IO dispatcher and
callers should marshal to their own executors if needed. Ensure you reference
observeNetworkRestore, networkManagerScope, networkMonitor.networkRestoreEvents,
and onRestored when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 92a97806-071e-43c1-893b-8ff98310b026
📒 Files selected for processing (6)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/events/EventQueueManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.ktclevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkMonitorTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkMonitorTest.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.kt
Base automatically changed from
task/SDK-4890-network-info-fetch
to
task/release/8.1.0
April 1, 2026 06:02
- When the device transitions from offline → online, the auto-flush drains all queued events from the DB. However, any previously scheduled timer-based flush (commsRunnable / pushNotificationViewedRunnable) would still fire and perform a redundant empty DB read. - Cancel those pending timers before auto-flushing since the network-restore flush already sends everything in the DB.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt`:
- Around line 85-94: The observeNetworkRestore registration is racy and won’t
allow re-registration after completion; wrap the networkRestoreJob check and
assignment in a short synchronized/lock block (use a private lock object or
synchronized(this)) and change the condition to create a new job when
networkRestoreJob is null or not active (e.g., networkRestoreJob == null ||
networkRestoreJob?.isActive == false); inside the launched job
(networkManagerScope.launch { networkMonitor.networkRestoreEvents.collect { ...
} }) attach invokeOnCompletion to set networkRestoreJob = null so the observer
can be restarted, and keep using config.accountId, logger.debug and onRestored
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5000ec5-aad6-4782-8caf-59848e7479af
📒 Files selected for processing (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.ktclevertap-core/src/main/java/com/clevertap/android/sdk/events/EventQueueManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.ktclevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.ktclevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkMonitor.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
Release Notes