Feature/profile image #75 #76
Conversation
…rovider, and permission_handler dependencies
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve updates to the Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (5)
android/app/src/main/AndroidManifest.xml (2)
7-10: Review permission declaration strategyThe combination of both modern (
READ_MEDIA_IMAGES) and legacy storage permissions is good for backward compatibility. However:
- Consider adding
maxSdkVersion="32"to legacy storage permissions to restrict them on newer Android versions- Document the minimum supported Android version in the README
- <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> - <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> + <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" android:maxSdkVersion="32" /> + <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="32" />
Line range hint
18-19:⚠️ Security: Exposed Google Maps API keyThe Google Maps API key is exposed in plain text in the manifest. This key should be protected to prevent unauthorized usage.
Recommendations:
- Move the API key to a secure location (e.g., encrypted properties file)
- Implement API key restrictions in Google Cloud Console
- Consider using different keys for debug and release builds
Would you like assistance in implementing a secure key management solution?
lib/features/profile/presentation/bloc/profile_bloc.dart (2)
Line range hint
73-83: Enhance error handling and loggingThe error handling could be more specific and provide better context to help with debugging.
Consider this improvement:
} catch (e) { - Logger().e('Failed to update profile: $e'); + Logger().e('Failed to update profile', error: e, stackTrace: StackTrace.current); emit( - ProfileError('Please connect to the internet to update your profile'), + ProfileError( + 'Failed to update profile: ${e is NetworkException ? 'Please check your internet connection' : 'An unexpected error occurred'}' + ), ); }
Line range hint
1-150: Consider standardizing error handling patternsThe error handling approach varies across different methods. Consider implementing a consistent error handling strategy across the bloc.
Suggestions:
- Create custom exceptions for different error scenarios (e.g.,
ProfileUpdateException,CacheException)- Implement a centralized error handling method
- Consider using a Result type for better error handling
Example implementation:
class ProfileException implements Exception { final String message; final dynamic originalError; ProfileException(this.message, [this.originalError]); @override String toString() => 'ProfileException: $message'; } Future<void> _handleError( String operation, dynamic error, Emitter<ProfileState> emit, ) { final message = _getErrorMessage(error); Logger().e('Failed to $operation', error: error, stackTrace: StackTrace.current); emit(ProfileError(message)); }lib/features/profile/presentation/screens/profile_screen.dart (1)
170-181: Enhance profile image UI with loading and error statesThe current implementation lacks loading indicators and error state handling.
Consider this improved implementation:
GestureDetector( onTap: _pickImage, - child: CircleAvatar( - radius: 50, - backgroundImage: _profileImage != null - ? FileImage(_profileImage!) - : null, - child: _profileImage == null - ? const Icon(Icons.camera_alt, size: 50) - : null, + child: Stack( + children: [ + CircleAvatar( + radius: 50, + backgroundImage: _profileImage != null + ? FileImage(_profileImage!) + : null, + onBackgroundImageError: (e, stack) { + Logger().e('Error loading profile image', error: e, stackTrace: stack); + }, + child: _profileImage == null + ? const Icon(Icons.camera_alt, size: 50) + : null, + ), + if (_isLoading) + const Positioned.fill( + child: CircularProgressIndicator(), + ), + ], ), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
android/app/src/main/AndroidManifest.xml(1 hunks)android/app/src/profile/AndroidManifest.xml(1 hunks)lib/features/profile/presentation/bloc/profile_bloc.dart(3 hunks)lib/features/profile/presentation/screens/profile_screen.dart(9 hunks)lib/features/support/presentation/bloc/support_bloc.dart(0 hunks)pubspec.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- lib/features/support/presentation/bloc/support_bloc.dart
🔇 Additional comments (2)
pubspec.yaml (1)
31-31: Verify dependency version compatibility
The new dependencies align with the profile image feature requirements. However:
- The
camerapackage version^0.11.0+2is quite old. Consider upgrading to the latest stable version for better Android 13+ support - Ensure
permission_handlerimplementation includes proper error handling and fallbacks
Let's check for the latest stable versions and any known security issues:
Also applies to: 53-53, 61-62
lib/features/profile/presentation/bloc/profile_bloc.dart (1)
4-4: LGTM: Required imports added for file operations
The new imports for dart:io and path_provider are correctly added to support profile image caching functionality.
Also applies to: 15-15
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> | ||
| <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> | ||
| <uses-permission android:name="android.permission.CAMERA" /> |
There was a problem hiding this comment.
💡 Codebase verification
Storage permissions need to be updated for modern Android compatibility
The verification reveals several issues that need to be addressed:
- The main AndroidManifest.xml already includes both legacy storage permissions and the modern
READ_MEDIA_IMAGESpermission, causing redundancy - The app uses
android:requestLegacyExternalStorage="true"which forces the legacy storage model - The permission handling code in
profile_screen.dartstill uses the deprecatedPermission.storageAPI
Recommendations:
- Remove
READ_EXTERNAL_STORAGEandWRITE_EXTERNAL_STORAGEfrom profile/AndroidManifest.xml as they're redundant - Remove
android:requestLegacyExternalStorage="true"from the application tag - Update the permission request in
profile_screen.dartto usePermission.photosinstead ofPermission.storage
🔗 Analysis chain
Consider using scoped storage and modern permission model
While the permissions are correctly declared, consider the following security recommendations:
- For Android 13+ (API 33+), prefer using
READ_MEDIA_IMAGESinstead of broad storage permissions - For Android 10+ (API 29+), consider implementing scoped storage instead of using legacy storage permissions
- Ensure runtime permission requests are properly implemented in the Flutter code
Let's verify the target SDK version and storage implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check build.gradle for targetSdkVersion and storage-related configurations
rg "targetSdkVersion|compileSdkVersion|requestLegacyExternalStorage" android/app/build.gradle
Length of output: 183
Script:
#!/bin/bash
# Check flutter.properties or local.properties for SDK version values
rg -l "compileSdkVersion|targetSdkVersion" android/
# Check main AndroidManifest.xml for storage-related configurations
cat android/app/src/main/AndroidManifest.xml
# Search for storage permission handling in Dart code
rg "requestPermission|getPermission|storage|imagePicker" lib/
Length of output: 3996
|
|
||
| final Directory cacheDir = await getTemporaryDirectory(); | ||
| final String imagePath = '${cacheDir.path}/profile_image.png'; | ||
| if (File(imagePath).existsSync()) { | ||
| // Handle cached profile image | ||
| } | ||
|
|
There was a problem hiding this comment.
Incomplete implementation of cached profile image handling
The code checks for the existence of a cached profile image but doesn't implement any handling logic. This could lead to inconsistent state management.
Consider implementing the image handling logic:
final Directory cacheDir = await getTemporaryDirectory();
final String imagePath = '${cacheDir.path}/profile_image.png';
if (File(imagePath).existsSync()) {
+ try {
+ final File cachedImage = File(imagePath);
+ // Handle the cached image, e.g., emit a state with the cached image
+ emit(ProfileLoaded(event.updatedProfile, cachedImagePath: imagePath));
+ } catch (e) {
+ Logger().e('Failed to handle cached profile image: $e');
+ }
- // Handle cached profile image
}Committable suggestion skipped: line range outside the PR's diff.
| if (_profileImage != null) { | ||
| // Save the profile image to cache | ||
| final Directory cacheDir = await getTemporaryDirectory(); | ||
| final String imagePath = '${cacheDir.path}/profile_image.png'; | ||
| await _profileImage!.copy(imagePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate profile saving logic and add cleanup
There's duplicate code for saving profile data to cache, and no cleanup of old cached files.
Consider extracting the caching logic to a separate method and adding cleanup:
+ Future<void> _saveToCache(UserProfile profile, {File? image}) async {
+ try {
+ final Directory cacheDir = await getTemporaryDirectory();
+
+ // Clean up old cache files
+ final cacheFiles = await cacheDir.list().where((entity) =>
+ entity.path.contains('profile')).toList();
+ for (var file in cacheFiles) {
+ await file.delete();
+ }
+
+ // Save new profile data
+ final String profilePath = '${cacheDir.path}/profile.json';
+ await File(profilePath).writeAsString(profile.toMap().toString());
+
+ // Save new image if provided
+ if (image != null) {
+ final String imagePath = '${cacheDir.path}/profile_image.png';
+ await image.copy(imagePath);
+ }
+ } catch (e) {
+ Logger().e('Error saving to cache', error: e, stackTrace: StackTrace.current);
+ throw Exception('Failed to save profile to cache');
+ }
+ }Update the profile saving logic to use this method:
if (internetConnectionBloc.state is DisconnectedInternet) {
- final Directory cacheDir = await getTemporaryDirectory();
- final String profilePath = '${cacheDir.path}/profile.json';
- await File(profilePath).writeAsString(updatedProfile.toMap().toString());
+ await _saveToCache(updatedProfile, image: _profileImage);
// ... show snackbar
}Also applies to: 231-251
| Future<void> _loadProfileImage() async { | ||
| final Directory cacheDir = await getTemporaryDirectory(); | ||
| final String imagePath = '${cacheDir.path}/profile_image.png'; | ||
| if (File(imagePath).existsSync()) { | ||
| setState(() { | ||
| _profileImage = File(imagePath); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling to profile image loading
The image loading implementation lacks error handling and proper cleanup.
Consider this improved implementation:
Future<void> _loadProfileImage() async {
+ try {
final Directory cacheDir = await getTemporaryDirectory();
final String imagePath = '${cacheDir.path}/profile_image.png';
- if (File(imagePath).existsSync()) {
+ final file = File(imagePath);
+ if (await file.exists()) {
setState(() {
_profileImage = File(imagePath);
});
}
+ } catch (e) {
+ Logger().e('Error loading profile image', error: e, stackTrace: StackTrace.current);
+ // Don't show error to user as this is a silent operation
}
}Committable suggestion skipped: line range outside the PR's diff.
| Future<void> _pickImage() async { | ||
| try { | ||
| // Check storage permission | ||
| if (await Permission.storage.request().isGranted) { | ||
| final ImagePicker picker = ImagePicker(); | ||
| final XFile? image = await picker.pickImage(source: ImageSource.gallery); | ||
|
|
||
| if (image != null) { | ||
| setState(() { | ||
| _profileImage = File(image.path); | ||
| }); | ||
|
|
||
| // Save the profile image to cache | ||
| final Directory cacheDir = await getTemporaryDirectory(); | ||
| final String imagePath = '${cacheDir.path}/profile_image.png'; | ||
| await _profileImage!.copy(imagePath); | ||
| } | ||
| } else { | ||
| print('Storage permission not granted.'); | ||
| } | ||
| } catch (e) { | ||
| print('Error picking image: $e'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve image picking implementation
Several improvements needed in the image picking implementation:
- Error handling uses
printinstead of proper logging - Missing image validation (size, format)
- Permission handling could be more robust
Consider this improved implementation:
Future<void> _pickImage() async {
try {
- // Check storage permission
- if (await Permission.storage.request().isGranted) {
+ final status = await Permission.storage.request();
+ if (status.isGranted) {
final ImagePicker picker = ImagePicker();
- final XFile? image = await picker.pickImage(source: ImageSource.gallery);
+ final XFile? image = await picker.pickImage(
+ source: ImageSource.gallery,
+ maxWidth: 1024,
+ maxHeight: 1024,
+ imageQuality: 80,
+ );
if (image != null) {
+ // Validate image size
+ final fileSize = await image.length();
+ if (fileSize > 5 * 1024 * 1024) { // 5MB limit
+ throw Exception('Image size too large. Please select an image under 5MB.');
+ }
+
setState(() {
_profileImage = File(image.path);
});
// Save the profile image to cache
final Directory cacheDir = await getTemporaryDirectory();
final String imagePath = '${cacheDir.path}/profile_image.png';
- await _profileImage!.copy(imagePath);
+ await _profileImage!.copy(imagePath).catchError((error) {
+ throw Exception('Failed to cache profile image: $error');
+ });
}
} else {
- print('Storage permission not granted.');
+ throw Exception('Storage permission required to select profile image');
}
} catch (e) {
- print('Error picking image: $e');
+ Logger().e('Error picking image', error: e, stackTrace: StackTrace.current);
+ if (mounted) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text(e.toString())),
+ );
+ }
}
}📝 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.
| Future<void> _pickImage() async { | |
| try { | |
| // Check storage permission | |
| if (await Permission.storage.request().isGranted) { | |
| final ImagePicker picker = ImagePicker(); | |
| final XFile? image = await picker.pickImage(source: ImageSource.gallery); | |
| if (image != null) { | |
| setState(() { | |
| _profileImage = File(image.path); | |
| }); | |
| // Save the profile image to cache | |
| final Directory cacheDir = await getTemporaryDirectory(); | |
| final String imagePath = '${cacheDir.path}/profile_image.png'; | |
| await _profileImage!.copy(imagePath); | |
| } | |
| } else { | |
| print('Storage permission not granted.'); | |
| } | |
| } catch (e) { | |
| print('Error picking image: $e'); | |
| } | |
| } | |
| Future<void> _pickImage() async { | |
| try { | |
| final status = await Permission.storage.request(); | |
| if (status.isGranted) { | |
| final ImagePicker picker = ImagePicker(); | |
| final XFile? image = await picker.pickImage( | |
| source: ImageSource.gallery, | |
| maxWidth: 1024, | |
| maxHeight: 1024, | |
| imageQuality: 80, | |
| ); | |
| if (image != null) { | |
| // Validate image size | |
| final fileSize = await image.length(); | |
| if (fileSize > 5 * 1024 * 1024) { // 5MB limit | |
| throw Exception('Image size too large. Please select an image under 5MB.'); | |
| } | |
| setState(() { | |
| _profileImage = File(image.path); | |
| }); | |
| // Save the profile image to cache | |
| final Directory cacheDir = await getTemporaryDirectory(); | |
| final String imagePath = '${cacheDir.path}/profile_image.png'; | |
| await _profileImage!.copy(imagePath).catchError((error) { | |
| throw Exception('Failed to cache profile image: $error'); | |
| }); | |
| } | |
| } else { | |
| throw Exception('Storage permission required to select profile image'); | |
| } | |
| } catch (e) { | |
| Logger().e('Error picking image', error: e, stackTrace: StackTrace.current); | |
| if (mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar(content: Text(e.toString())), | |
| ); | |
| } | |
| } | |
| } |
This pull request includes several changes to enhance the profile image handling functionality and update permissions in the Android manifest files. The most important changes include adding new permissions, updating the profile bloc to handle profile images, and modifying the profile screen to allow users to pick and display profile images.
Permissions Update:
android/app/src/main/AndroidManifest.xml: Added permissions for reading and writing external storage, accessing the camera, and reading media images.android/app/src/profile/AndroidManifest.xml: Added permissions for reading and writing external storage and accessing the camera.Profile Bloc Enhancements:
lib/features/profile/presentation/bloc/profile_bloc.dart: Importeddart:ioandpath_providerpackages, and added logic to handle cached profile images. [1] [2] [3]Profile Screen Modifications:
lib/features/profile/presentation/screens/profile_screen.dart: Imported necessary packages for image picking and permission handling, and added methods to pick, load, and display profile images. [1] [2] [3] [4] [5] [6] [7] [8]Dependency Updates:
pubspec.yaml: Added dependencies forcamera,image_picker,path_provider, andpermission_handlerpackages. [1] [2]Minor Changes:
lib/features/support/presentation/bloc/support_bloc.dart: Removed an unnecessary comment.Summary by CodeRabbit
Release Notes
New Features
Permissions
Dependencies