Upgrade image_cropper from v5.0.1 to v8.0.2 with updated implementations and tests
#2697
Conversation
WalkthroughThis pull request focuses on upgrading the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Finishing Touches
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/services/image_service.dart (1)
41-44: Consider adding more aspect ratio presets for better flexibility.The current implementation only includes
squareandoriginalpresets. Consider adding more common presets likeratio3x2,ratio4x3,ratio16x9to provide users with more cropping options.aspectRatioPresets: [ CropAspectRatioPreset.square, CropAspectRatioPreset.original, + CropAspectRatioPreset.ratio3x2, + CropAspectRatioPreset.ratio4x3, + CropAspectRatioPreset.ratio16x9, ],Also applies to: 48-51
test/service_tests/image_service_test.dart (1)
70-112: Enhance aspect ratio preset tests.While the current test verifies the presence of presets, consider adding tests for:
- The order of presets (if order matters)
- Negative scenarios (invalid presets)
- Platform-specific preset behaviors
test/service_tests/multi_media_pick_service_test.dart (1)
88-117: Consider expanding aspect ratio preset coverage.The test only verifies
squareandoriginalpresets. Consider adding assertions for other commonly used presets likeratio3x2,ratio4x3,ratio16x9to ensure comprehensive coverage.Example addition:
expect( androidSettings.aspectRatioPresets, contains(CropAspectRatioPreset.square), ); expect( androidSettings.aspectRatioPresets, contains(CropAspectRatioPreset.original), ); +expect( + androidSettings.aspectRatioPresets, + contains(CropAspectRatioPreset.ratio3x2), +); +expect( + androidSettings.aspectRatioPresets, + contains(CropAspectRatioPreset.ratio4x3), +); +expect( + androidSettings.aspectRatioPresets, + contains(CropAspectRatioPreset.ratio16x9), +);Similarly for iOS settings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
lib/services/image_service.dart(1 hunks)pubspec.yaml(1 hunks)test/helpers/test_helpers.mocks.dart(0 hunks)test/service_tests/image_service_test.dart(2 hunks)test/service_tests/multi_media_pick_service_test.dart(2 hunks)
💤 Files with no reviewable changes (1)
- test/helpers/test_helpers.mocks.dart
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (4)
pubspec.yaml (2)
Line range hint
15-19: Verify impact of removed analyzer dependencies.The removal of analyzer-related dependencies needs verification to ensure it doesn't affect the project's static analysis capabilities.
Also applies to: 23-24
54-54: Verify the image_cropper version upgrade.The version jump from 5.0.1 to 8.0.2 is significant. Let's verify:
- If there are any security advisories
- If this is the latest stable version
✅ Verification successful
The image_cropper upgrade to ^8.0.2 appears safe to proceed.
The implementation uses stable core features, and there are no security advisories. The version is close to the latest (8.1.0), and the codebase uses standard cropping functionality that is unlikely to be affected by breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check PyPI for latest versions and security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PUB, package: "image_cropper") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check pub.dev for latest version curl -s https://pub.dev/packages/image_cropper.json | jq '.latest.version'Length of output: 425
Script:
#!/bin/bash # Try alternative pub.dev API endpoint curl -s "https://pub.dartlang.org/api/packages/image_cropper" | jq -r '.versions[] | .version' | sort -V # Check image_cropper usage in the codebase rg "image_cropper" --type dart -lLength of output: 727
Script:
#!/bin/bash # Check image_cropper usage in the service cat lib/services/image_service.dart # Look for any migration guides or breaking changes in README cat $(fd -t f "README.md" -x echo {})Length of output: 12785
test/service_tests/multi_media_pick_service_test.dart (2)
20-31: LGTM! Test setup changes improve isolation.The changes follow testing best practices:
- Moving
registerServices()tosetUpAllis more efficient- Resetting the mock before each test prevents interference
71-87: LGTM! Well-structured test for breaking changes.The test properly verifies the migration of
aspectRatioPresetsto platform-specific UI settings, which is crucial for the v7.0.0+ breaking changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2697 +/- ##
=================================================
Coverage 96.55% 96.55%
=================================================
Files 189 189
Lines 9994 9995 +1
=================================================
+ Hits 9650 9651 +1
Misses 344 344 ☔ View full report in Codecov by Sentry. |
|
The image_cropper plugin requires compileSdkVersion 35 or higher, which in turn necessitates:
Upgrading Gradle and AGP to these versions might introduce breaking changes, as:
For iOS, the Podfile.lock needs to be updated as the current snapshots are not matching, potentially indicating dependency mismatches or conflicts. Kindly provide guidance on the following:
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
android/app/build.gradle (1)
Line range hint
36-47: Additional updates required for Gradle compatibility.The SDK version upgrade to 35 requires:
- Gradle version 8.9 or higher
- Android Gradle Plugin (AGP) version 8.6.0 or higher
- Namespace changes for Gradle 8+
Please update the following files:
android/build.gradle:
- Update Gradle version
- Update AGP version
android/app/build.gradle:
- Add mandatory namespace configurations
Would you like me to provide the specific changes needed for these updates?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/app/build.gradle(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/pull-request.yml (1)
Line range hint
37-233: Consider adding Gradle version verification.Given that image_cropper v8.0.2 requires specific Gradle versions, consider adding a step to verify Gradle version compatibility in the Android build jobs.
Add this step after the Flutter setup:
- uses: subosito/flutter-action@v2 with: flutter-version: "3.24.4" channel: "stable" + - name: Verify Gradle Version + run: | + cd android + ./gradlew --versiontest/service_tests/image_service_test.dart (3)
55-68: Consider using a more realistic test file path.While the test logic is solid, consider using a path that matches your actual application's file structure (e.g., 'test/fixtures/images/test.png'). This makes the test more representative of real-world scenarios.
70-136: Refactor repetitive assertions for better maintainability.While the test is thorough, the repetitive expect statements could be refactored for better maintainability. Consider using a helper method or a list of expected presets.
Here's a suggested refactor:
void _verifyAspectRatioPresets(PlatformUiSettings settings) { final expectedPresets = [ CropAspectRatioPreset.square, CropAspectRatioPreset.original, CropAspectRatioPreset.ratio3x2, CropAspectRatioPreset.ratio4x3, CropAspectRatioPreset.ratio16x9, ]; for (final preset in expectedPresets) { expect( settings.aspectRatioPresets, contains(preset), reason: 'Missing preset: $preset', ); } } // Usage in test: _verifyAspectRatioPresets(androidSettings); _verifyAspectRatioPresets(iosSettings);
Line range hint
138-152: Consider using a more specific exception type.The test currently throws and expects a generic Exception. Consider using a more specific exception type that matches the actual error scenarios in your application.
Example:
when( mockImageCropper.cropImage( sourcePath: "test", uiSettings: anyNamed('uiSettings'), ), ).thenThrow(ImageCropperException('Failed to crop image')); // Use specific exception expect( imageService.cropImage(imageFile: fakefile), throwsA(isA<ImageCropperException>()), // Match specific exception );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/pull-request.yml(4 hunks).github/workflows/push.yml(4 hunks)android/app/build.gradle(4 hunks)android/gradle/wrapper/gradle-wrapper.properties(1 hunks)android/settings.gradle(1 hunks)lib/services/image_service.dart(1 hunks)pubspec.yaml(3 hunks)test/service_tests/image_service_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/services/image_service.dart
- android/app/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (9)
.github/workflows/push.yml (1)
43-43: Verify Flutter version compatibility with required build tools.The Flutter version has been updated to 3.24.4 across all jobs. However, based on the PR objectives, we need to ensure this version supports:
- Gradle 8.9+ and Android Gradle Plugin (AGP) 8.6.0+ (required by image_cropper v8.0.2)
- Android SDK 35+ (required by image_cropper v8.0.2)
Let's verify the compatibility:
Also applies to: 88-88, 120-120, 153-153
✅ Verification successful
Flutter 3.24.4 is compatible with the project's build configuration
- Verified compatibility with Android SDK 35
- Confirmed support for Java 17 and Kotlin 1.8.10
- Flutter 3.24.4 supports the Gradle 8.9+ and AGP 8.6.0+ requirements from image_cropper v8.0.2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Flutter version details and Android toolchain requirements # Check Flutter version details flutter --version # Check Android toolchain requirements in flutter.gradle cat $(fd -t f flutter.gradle$ | head -n 1) # Check build.gradle for AGP version fd -t f "build.gradle$" -x catLength of output: 2784
.github/workflows/pull-request.yml (1)
37-37: Consistent Flutter version update across workflows.The Flutter version has been consistently updated to 3.24.4 across all jobs in both workflow files, which is good practice.
Also applies to: 187-187, 218-218, 233-233
test/service_tests/image_service_test.dart (2)
25-35: Well-structured mock setup!Good practice to reset the mock before each test to prevent any interference between test cases.
Line range hint
1-180: Verify handling of breaking changes from image_cropper v7.0.0.The tests have been updated to handle the UI settings changes, but let's verify all breaking changes from v7.0.0 are covered.
✅ Verification successful
All breaking changes from image_cropper v7.0.0 are properly handled
The implementation and tests correctly handle:
- Updated cropImage signature with uiSettings parameter
- New CroppedFile return type
- Platform-specific UI settings for both Android and iOS
- Comprehensive error cases and null scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to old image_cropper API rg -l "aspectRatioPresets.*cropImage" --type dart # Check for any TODO comments related to image_cropper upgrade rg -i "todo.*image.*cropper" --type dartLength of output: 94
Script:
#!/bin/bash # Find ImageService implementation ast-grep --pattern 'class ImageService { $$$ cropImage($$$) { $$$ } $$$ }' # Find direct usage of cropImage method rg "cropImage\(" -A 3 --type dartLength of output: 5254
android/gradle/wrapper/gradle-wrapper.properties (1)
6-6: Verify Gradle 8.9 compatibility with all dependenciesThe upgrade to Gradle 8.9 is significant and introduces breaking changes:
- Requires adding namespace to package definitions
- May affect dependencies that don't support Gradle 8.x
Run this script to check for potential compatibility issues:
✅ Verification successful
Gradle 8.9 compatibility requirements are satisfied ✅
The namespace requirement is properly configured in
android/app/build.gradleand the current dependencies are compatible with Gradle 8.9:
- Namespace is set to "com.example.talawa"
- All dependencies are using stable, modern versions
- No deprecated packages detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check build.gradle files for namespace definitions and potential compatibility issues echo "Checking for namespace definitions in build.gradle files..." rg -l "namespace" android/app/build.gradle || echo "Warning: No namespace found in build.gradle" echo "\nChecking for potentially affected dependencies..." rg "dependencies\s*{" android/app/build.gradle -A 20Length of output: 460
Script:
#!/bin/bash # Check namespace and dependencies in build files echo "=== Checking build.gradle files ===" fd build.gradle$ --type f -E node_modules -E build -E dist | while read -r file; do echo "\nFile: $file" echo "--- Namespace check ---" rg "namespace" "$file" || echo "No namespace found" echo "--- Dependencies check ---" rg "dependencies.*\{" "$file" -A 10 || echo "No dependencies block found" done echo "\n=== Checking for migration guides ===" fd -t f -e md -e txt | xargs rg -l "gradle.*migration|upgrade.*gradle" || echo "No migration guides found"Length of output: 1333
pubspec.yaml (4)
54-54: Handle breaking changes in image_cropper v8.0.2The upgrade from v5.0.1 to v8.0.2 includes breaking changes in the
cropImage()function, particularly regarding AndroidUiSettings and IOUiSettings.
13-13: Verify Flutter SDK compatibilityThe SDK version constraint has been updated to ">=2.17.0 <=3.5.4". Ensure this aligns with the Flutter version used in CI/CD.
✅ Verification successful
SDK version constraint is compatible with CI/CD configuration ✓
The SDK constraint
>=2.17.0 <=3.5.4in pubspec.yaml is compatible with Flutter version 3.24.4 used in CI/CD workflows.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Flutter SDK version in CI/CD configuration echo "Checking Flutter version in CI/CD..." rg "flutter-version" .github/workflows/ -A 1Length of output: 1412
Line range hint
16-19: Verify impact of removed analyzer dependenciesThe following analyzer dependencies have been removed:
- _fe_analyzer_shared
- analyzer
- analyzer_plugin
- custom_lint_builder
Also applies to: 29-31
81-81: Verify necessity of win32 dependencyThe new win32 dependency (^5.10.0) has been added. Please clarify if this is required for the image_cropper upgrade or if it serves another purpose.
|
I have resolved the iOS issues, and the tests are now passing for the iOS build. 🎉 Breaking ChangeThis PR introduces a breaking change as it involves modifications to multiple files in the android folder. Below are the key updates and their rationale: Gradle and AGP Updates
Package Updates
@palisadoes @noman2002 Or is there an alternative approach that you would suggest for handling this situation? Looking forward to your input. |
|
Please do the required upgrades. This is a security vulnerability. |
|
@palisadoes @noman2002 |
@noman2002 I havent changed any files , only whitespaces have been fixed and double "" quotes been added automatically. Still I have reverted all the changes. |
may-tas
force-pushed
the
migrate-image-cropper
branch
from
37197b0 to
c586027
Compare
What kind of change does this PR introduce?
image_cropperto v8.0.2Issue Number:
image_cropperfrom 5.0.1 to 8.0.2 #2554Did you add tests for your changes?
Snapshots/Videos:
Summary
image_cropperfrom v5.0.1 to v8.0.2, updated implementation to meet new version requirements, and fixed related test cases.Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Dependency Updates
image_cropperdependency to version 8.0.2.Platform Improvements
Performance