-
Notifications
You must be signed in to change notification settings - Fork 0
Introduce a network monitor into core
#22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
corecore
corecore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a network monitor into the core module as a prerequisite for connection recovery. The implementation provides real-time network connectivity information including transport types, signal strength, bandwidth estimates, and connection properties.
Key changes:
- Adds
StreamNetworkMonitorinterface with internal implementation that observes Android's ConnectivityManager - Exposes
networkInfovia StreamClient as an internal API - Introduces
StreamAndroidComponentsProviderfor dependency injection of Android system services
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| StreamSocketSessionTest.kt | Updated mock for health monitor to return Result instead of Unit |
| Multiple test files (StreamNetwork*.kt) | Added comprehensive test coverage for network monitoring components |
| AlgebraTest.kt | Tests for new exception and Result combination operators |
| StreamHealthMonitorImpl.kt | Changed return types from Unit to Result |
| StreamNetworkSnapshotBuilder.kt | Builds network capability snapshots from Android system data |
| StreamNetworkSignalProcessing.kt | Processes WiFi and cellular signal strength information |
| StreamNetworkMonitorUtils.kt | Utility functions for safe capability checks |
| StreamNetworkMonitorImpl.kt | Core implementation of network monitoring |
| StreamNetworkMonitorCallback.kt | Handles ConnectivityManager callbacks |
| StreamAndroidComponentsProviderImpl.kt | Implementation for accessing Android system services |
| StreamClientImpl.kt | Integrates network monitor into client lifecycle |
| Algebra.kt | Defines operators for combining exceptions and Results |
| StreamHealthMonitor.kt | Updated interface with Result return types |
| StreamNetworkMonitorListener.kt | Listener interface for network state changes |
| StreamNetworkMonitor.kt | Public API and factory function for network monitor |
| StreamNetworkInfo.kt | Data models for network state snapshots |
| StreamAndroidComponentsProvider.kt | Interface for accessing Android system services |
| StreamClient.kt | Added networkInfo property and networkMonitor parameter |
| AndroidManifest.xml | Added required network permissions |
| build.gradle.kts | Added androidx.annotation dependency |
| libs.versions.toml | Added annotation-jvm library version |
| Sample app files | UI components and integration for displaying network info |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
stream-android-core/src/main/java/io/getstream/android/core/api/utils/Algebra.kt
Outdated
Show resolved
Hide resolved
stream-android-core/src/main/java/io/getstream/android/core/internal/client/StreamClientImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ndroid-core/src/test/java/io/getstream/android/core/internal/client/StreamClientIImplTest.kt
Show resolved
Hide resolved
...e/src/main/java/io/getstream/android/core/internal/socket/monitor/StreamHealthMonitorImpl.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I left just a few nitpicks
| * | ||
| * @param snapshot A [StreamNetworkInfo.Snapshot] describing the newly connected network. | ||
| */ | ||
| public suspend fun onNetworkConnected(snapshot: StreamNetworkInfo.Snapshot?) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the parameter non-nullable? The only usage I see is after a null check.
With that we can also make the snapshot in StremNetworkState.Available non-nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Snapshot is not available pre API 24 and our min is API 21, its either this, or having a separate callback without the snapshot for lower APIs.
Do you have any other suggestion?
| object : StreamNetworkMonitorListener { | ||
| override suspend fun onNetworkConnected( | ||
| snapshot: StreamNetworkInfo.Snapshot? | ||
| ) { | ||
| logger.v { | ||
| "[connect] Network connected: $snapshot" | ||
| } | ||
| val state = StreamNetworkState.Available(snapshot) | ||
| mutableNetworkState.update(state) | ||
| subscriptionManager.forEach { | ||
| it.onNetworkState(state) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun onNetworkLost(permanent: Boolean) { | ||
| logger.v { "[connect] Network lost" } | ||
| val state = | ||
| if (permanent) { | ||
| StreamNetworkState.Unavailable | ||
| } else { | ||
| StreamNetworkState.Disconnected | ||
| } | ||
| mutableNetworkState.update(state) | ||
| subscriptionManager.forEach { | ||
| it.onNetworkState(state) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun onNetworkPropertiesChanged( | ||
| snapshot: StreamNetworkInfo.Snapshot | ||
| ) { | ||
| logger.v { "[connect] Network changed: $snapshot" } | ||
| mutableNetworkState.update( | ||
| StreamNetworkState.Available(snapshot) | ||
| ) | ||
| subscriptionManager.forEach { | ||
| it.onNetworkState( | ||
| StreamNetworkState.Available(snapshot) | ||
| ) | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd extract this to a function that creates the listener, just for clarity
...in/java/io/getstream/android/core/internal/observers/network/StreamNetworkMonitorCallback.kt
Outdated
Show resolved
Hide resolved
…ternal/observers/network/StreamNetworkMonitorCallback.kt Co-authored-by: Gianmarco <[email protected]>
|



Goal
AND791
Introduce a network monitor into core, as a pre-requisite for implementing connection recovery.
Implementation
Network Monitor
StreamNetworkMonitorinterface. An network monitor supplied externally to theStreamClient. Internal implementation inStreamNetworkMonitorImplExposed
networkStatevia the client andonNetworkStatein the client listener.Components provider
In order to avoid depending on
Contextdirectly and be able to inject dependencies we are providing aStreamAndroidComponentsProviderwhich gives us the likes ofConnectivityManageretc..Other
HealthMonitorchanged return type to beResult<*>Added
Algebra.ktwhere operations are written e.g.Exception + Exceptionwill give a combined exception.Testing
Checklist