Add connectivity management with connectivity_plus package #2325
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the mobile application’s connectivity handling by adding a new permission in the AndroidManifest.xml, integrating the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (4)
mobile-v3/lib/src/app/shared/bloc/connectivity_bloc.dart (2)
35-42: Consider a more robust method for checking internet connectivityUsing
InternetAddress.lookup('google.com')can be unreliable and may cause delays, especially in regions where 'google.com' is inaccessible. Consider using a lightweight HTTP request or leveraging the connectivity_plus package's capabilities to check for internet access.Example using a simple HTTP request:
import 'package:http/http.dart' as http; Future<bool> _hasInternetConnection() async { try { final result = await http.get(Uri.parse('https://www.google.com')); return result.statusCode == 200; } catch (e) { return false; } }
20-24: Handle potential exceptions in connectivity subscriptionWhile setting up the connectivity subscription, it's good practice to handle potential exceptions that might occur during the stream listening.
Consider wrapping the subscription in a try-catch block to handle any unexpected errors:
try { _connectivitySubscription = _connectivity.onConnectivityChanged.listen((result) { debugPrint('Connectivity changed: $result'); add(ConnectivityChanged(result != ConnectivityResult.none)); }); } catch (e) { debugPrint('Error listening to connectivity changes: $e'); }mobile-v3/lib/src/app/shared/pages/no_internet_banner.dart (1)
26-36: Consider adding more informative error message.While the current implementation is functional, consider providing more detailed information or guidance to users when offline.
Expanded( child: Text( - 'Internet Connection Lost', + 'Internet Connection Lost - Please check your network settings', style: TextStyle( color: Colors.white, fontWeight: FontWeight.bold, fontSize: 16.0, ), overflow: TextOverflow.ellipsis, ), ),mobile-v3/lib/main.dart (1)
141-177: Consider improving error handling messageThe connectivity state management implementation looks good, but the error message could be more user-friendly.
Consider updating the error message with more context:
- body: Center(child: Text('An Error occurred.'))); + body: Center( + child: Text( + 'Unable to verify authentication status. Please try again.', + textAlign: TextAlign.center, + style: Theme.of(context).textTheme.bodyLarge, + ), + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mobile-v3/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
mobile-v3/android/app/src/main/AndroidManifest.xml(1 hunks)mobile-v3/android/app/src/main/java/io/flutter/plugins/GeneratedPluginRegistrant.java(1 hunks)mobile-v3/ios/Runner/GeneratedPluginRegistrant.m(2 hunks)mobile-v3/lib/main.dart(4 hunks)mobile-v3/lib/src/app/shared/bloc/connectivity_bloc.dart(1 hunks)mobile-v3/lib/src/app/shared/bloc/connectivity_event.dart(1 hunks)mobile-v3/lib/src/app/shared/bloc/connectivity_state.dart(1 hunks)mobile-v3/lib/src/app/shared/pages/no_internet_banner.dart(1 hunks)mobile-v3/pubspec.yaml(1 hunks)
🔇 Additional comments (11)
mobile-v3/lib/src/app/shared/bloc/connectivity_state.dart (1)
1-18: State classes are well-structured and adhere to best practices
The connectivity state classes are appropriately defined and extend Equatable, which is excellent for ensuring efficient state comparisons.
mobile-v3/lib/src/app/shared/bloc/connectivity_event.dart (1)
1-19: Event classes are properly defined and maintain consistency
The event classes extend Equatable, allowing for accurate event comparison. This setup aligns well with the BLoC pattern conventions.
mobile-v3/android/app/src/main/AndroidManifest.xml (1)
4-4: LGTM! Essential permission for network monitoring.
The ACCESS_NETWORK_STATE permission is correctly added and is required for the connectivity_plus package to monitor network status changes.
mobile-v3/android/app/src/main/java/io/flutter/plugins/GeneratedPluginRegistrant.java (1)
18-22: Well-structured error handling for ConnectivityPlugin.
The implementation follows the established pattern and includes proper error logging for debugging purposes.
mobile-v3/lib/src/app/shared/pages/no_internet_banner.dart (2)
6-8: Good improvement in callback handling.
Making onClose nullable provides better flexibility for widget usage.
12-18: Clean state management implementation.
The BlocBuilder integration is well-structured, with proper state type checking and conditional rendering.
mobile-v3/pubspec.yaml (1)
56-56: LGTM! Verify latest stable version of connectivity_plus
The addition of connectivity_plus package looks good. Let's verify we're using the latest stable version.
✅ Verification successful
You're using the latest version of connectivity_plus! 👍
The package version specified (^6.1.0) is already at the latest stable version available on pub.dev. Good job staying up to date!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version of connectivity_plus
curl -s https://pub.dev/packages/connectivity_plus.json | jq -r '.latest.version'
Length of output: 86
Script:
#!/bin/bash
# Let's try an alternative approach to check the package version
# First, let's verify the package exists and its version history
curl -s "https://pub.dev/api/packages/connectivity_plus" | jq -r '.versions[] | .version' | sort -V
Length of output: 480
mobile-v3/lib/main.dart (4)
20-20: LGTM! Required imports added correctly
The new imports for connectivity management are properly organized and necessary for the implementation.
Also applies to: 24-24, 30-30
73-73: LGTM! Proper initialization of connectivity management
The Connectivity instance is correctly initialized and properly provided through the BlocProvider. Good job on maintaining clean architecture principles.
Also applies to: 101-103
166-172: Verify NoInternetBanner implementation
Let's ensure the NoInternetBanner widget is properly implemented.
✅ Verification successful
NoInternetBanner implementation looks good
The implementation is properly structured with:
- NoInternetBanner widget correctly handles the onClose callback
- ConnectivityBloc properly manages the banner state
- ConnectivityBannerDismissed event is correctly implemented and handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NoInternetBanner implementation
rg -l "class NoInternetBanner" --type dart
Length of output: 103
Script:
#!/bin/bash
# Check NoInternetBanner implementation details
rg "class NoInternetBanner" -A 10 --type dart
# Check ConnectivityBloc implementation
rg "class ConnectivityBloc" -A 10 --type dart
# Check ConnectivityBannerDismissed event
rg "ConnectivityBannerDismissed" -A 5 --type dart
Length of output: 4133
141-144: Verify ConnectivityBloc implementation
Let's ensure the ConnectivityBloc is properly handling all connectivity states.
✅ Verification successful
Let me search for the implementation files first, and then we can analyze the bloc implementation in detail.
ConnectivityBloc implementation is properly handling connectivity states
The ConnectivityBloc implementation is robust and well-structured:
- Handles all connectivity states (Initial, Online, Offline)
- Properly manages connectivity changes through StreamSubscription
- Includes internet reachability check via
_hasInternetConnection() - Implements proper cleanup in
close()method - Handles banner dismissal state
- Uses equatable for state comparison
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ConnectivityBloc implementation
ast-grep --pattern 'class ConnectivityBloc extends Bloc<$_, $_> {
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for connectivity-related bloc files
fd -e dart | xargs rg -l "ConnectivityBloc" -A 5
# Also search for the state implementation
fd -e dart | xargs rg -l "ConnectivityState"
# And search for any connectivity-related event classes
fd -e dart | xargs rg -l "ConnectivityEvent"
Length of output: 591
Script:
#!/bin/bash
# Let's examine the actual implementation of the ConnectivityBloc
cat mobile-v3/lib/src/app/shared/bloc/connectivity_bloc.dart
# Check the state implementation
cat mobile-v3/lib/src/app/shared/bloc/connectivity_state.dart
# And the events
cat mobile-v3/lib/src/app/shared/bloc/connectivity_event.dart
Length of output: 3347
| Stream<ConnectivityState> mapEventToState(ConnectivityEvent event) async* { | ||
| if (event is ConnectivityChanged) { | ||
| yield event.isConnected ? ConnectivityOnline() : ConnectivityOffline(); | ||
| } else if (event is ConnectivityBannerDismissed) { | ||
| _bannerDismissed = true; | ||
| yield state; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mixing on<Event> and mapEventToState in the same Bloc
Using both on<Event> handlers and overriding mapEventToState can lead to unexpected behavior. It's recommended to use one pattern consistently. Since you're already using on<Event> handlers, you can remove the mapEventToState method.
Apply this diff to remove the mapEventToState method:
...
- Stream<ConnectivityState> mapEventToState(ConnectivityEvent event) async* {
- if (event is ConnectivityChanged) {
- yield event.isConnected ? ConnectivityOnline() : ConnectivityOffline();
- } else if (event is ConnectivityBannerDismissed) {
- _bannerDismissed = true;
- yield state;
- }
- }Committable suggestion skipped: line range outside the PR's diff.
| #if __has_include(<connectivity_plus/ConnectivityPlusPlugin.h>) | ||
| #import <connectivity_plus/ConnectivityPlusPlugin.h> | ||
| #else | ||
| @import connectivity_plus; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Avoid manual modifications to generated files
GeneratedPluginRegistrant.m is an auto-generated file. Manual edits might be overwritten. Ensure that the connectivity_plus plugin is correctly added to your pubspec.yaml, and let the Flutter tooling handle the plugin registration.
To include the plugin properly, verify that you've added it to pubspec.yaml:
dependencies:
connectivity_plus: ^6.1.0Then, run flutter pub get and rebuild the project to auto-generate the correct registrant files.
Also applies to: 30-30
| IconButton( | ||
| icon: Icon(Icons.close, color: Colors.white), | ||
| onPressed: () { | ||
| context.read<ConnectivityBloc>().add(ConnectivityBannerDismissed()); | ||
| onClose?.call(); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
💡 Codebase verification
Banner dismiss state does not persist across app restarts
The _bannerDismissed is initialized as a simple boolean variable without any persistence mechanism. It's set to false when the bloc is created and only stored in memory. When the app restarts, this state will be reset to its default value.
mobile-v3/lib/src/app/shared/bloc/connectivity_bloc.dart: The_bannerDismissedfield is initialized tofalseand lacks any persistence implementation.- No SharedPreferences or other storage mechanisms are used to save this state.
🔗 Analysis chain
Verify dismiss behavior persistence.
The banner dismissal triggers both local and bloc state updates. We should verify if this state persists across app restarts.
The previous pattern didn't yield results, so I'm broadening the search to understand the connectivity-related state management and persistence mechanisms. This will help us verify if the dismiss state is actually persisted.
I noticed a _bannerDismissed variable being set in the previous output. Let's examine how this variable is initialized and managed to determine if the state persists across app restarts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for persistence-related code in the ConnectivityBloc
ast-grep --pattern 'class ConnectivityBloc extends Bloc<ConnectivityEvent, ConnectivityState> {
$$$
isDismissed
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Let's search for the ConnectivityBloc implementation and state handling
rg -l "ConnectivityBloc" --type dart
# Then look for persistence or storage related code in connectivity
rg "ConnectivityBannerDismissed|isDismissed" --type dart -C 3
# Search for any shared preferences or storage related to connectivity
rg "SharedPreferences.*connect" --type dart -C 2
Length of output: 4254
Script:
#!/bin/bash
# Let's check the ConnectivityBloc class implementation for any persistence variables
rg "_bannerDismissed" --type dart -C 5
# Also check if there's any initialization code that might restore state
cat mobile-v3/lib/src/app/shared/bloc/connectivity_bloc.dart
Length of output: 5104
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
connectivity_plusfor enhanced connectivity features.