Upgrade image_cropper from v5.0.1 to v8.0.2 with updated implementations and tests #2711
Conversation
WalkthroughThe 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)
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: 2
🔭 Outside diff range comments (2)
ios/Runner/Info.plist (1)
Line range hint
1-72: Fix duplicate LSApplicationQueriesSchemes key.There are two instances of the LSApplicationQueriesSchemes key in the plist file. This could lead to unexpected behavior.
Merge the schemes into a single array:
<key>LSApplicationQueriesSchemes</key> <array> <string>instagram-stories</string> <string>facebook-stories</string> <string>facebook</string> <string>instagram</string> <string>twitter</string> <string>whatsapp</string> <string>tg</string> - </array> - <key>LSApplicationQueriesSchemes</key> - <array> <string>sms</string> <string>tel</string> </array>pubspec.yaml (1)
Custom lint dependencies should not be removed
The analyzer-related dependencies cannot be safely removed as they are required by the
talawa_lintcustom rules package:
- The
talawa_lintpackage (intalawa_lint/directory) directly depends on these packages- The project's
analysis_options.yamlactively uses the custom lint rules- Removing these dependencies would break the custom lint functionality
🔗 Analysis chain
Line range hint
16-19: Verify removal of analyzer dependencies.Please verify that the removed analyzer dependencies are not required by any other packages or development tools in the project.
Also applies to: 24-26
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any remaining files reference the removed packages echo "Checking for references to removed packages..." rg -l "_fe_analyzer_shared|analyzer[^_]|analyzer_plugin|custom_lint_builder" --type yaml --type dartLength of output: 647
Script:
#!/bin/bash # Check the contents of relevant files echo "=== talawa_lint/pubspec.yaml ===" cat talawa_lint/pubspec.yaml echo -e "\n=== analysis_options.yaml ===" cat analysis_options.yaml echo -e "\n=== Main pubspec.yaml analyzer references ===" rg "analyzer|custom_lint" pubspec.yamlLength of output: 3093
🧹 Nitpick comments (3)
lib/services/image_service.dart (1)
41-47: Consider making aspectRatioPresets configurable.The aspectRatioPresets are currently hardcoded. Consider making them configurable through method parameters to support different use cases.
-Future<File?> cropImage({required File imageFile}) async { +Future<File?> cropImage({ + required File imageFile, + List<CropAspectRatioPreset>? aspectRatioPresets, +}) async {Also applies to: 51-57
test/service_tests/image_service_test.dart (1)
96-115: Consider using a helper method for aspect ratio preset verification.The aspect ratio preset verification is repeated multiple times. Consider extracting it into a helper method to improve maintainability.
void verifyAspectRatioPresets(List<CropAspectRatioPreset> presets) { for (final preset in [ CropAspectRatioPreset.square, CropAspectRatioPreset.original, CropAspectRatioPreset.ratio3x2, CropAspectRatioPreset.ratio4x3, CropAspectRatioPreset.ratio16x9, ]) { expect(presets, contains(preset)); } }pubspec.yaml (1)
54-54: Consider using a caret version constraint.The strict version constraint
8.0.2might cause dependency resolution issues. Consider using^8.0.2to allow compatible minor and patch updates.- image_cropper: 8.0.2 + image_cropper: ^8.0.2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
android/settings.gradle(1 hunks)ios/Flutter/AppFrameworkInfo.plist(1 hunks)ios/Runner.xcodeproj/project.pbxproj(11 hunks)ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme(1 hunks)ios/Runner/Info.plist(1 hunks)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
✅ Files skipped from review due to trivial changes (2)
- ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
- android/settings.gradle
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (6)
ios/Flutter/AppFrameworkInfo.plist (1)
24-24: Verify iOS deployment target compatibility.The minimum iOS version has been updated from 9.0 to 12.0 to support image_cropper v8.0.2. This change may affect users with older iOS devices.
Consider adding this compatibility requirement to the app's documentation to inform users and developers about the minimum supported iOS version.
ios/Runner/Info.plist (1)
67-70: LGTM! Required iOS configuration keys added.The new configuration keys enhance iOS compatibility:
- CADisableMinimumFrameDurationOnPhone: Improves frame rate handling
- UIApplicationSupportsIndirectInputEvents: Enables proper input event handling
ios/Runner.xcodeproj/project.pbxproj (4)
6-6: LGTM! Project and build settings updated.The Xcode project has been properly updated:
- objectVersion increased to 54
- LastUpgradeCheck updated to 1510
Also applies to: 160-160
364-364: LGTM! Deployment target consistently updated.The iOS deployment target has been updated to 12.0 across all build configurations (Profile, Debug, Release), maintaining consistency with the image_cropper v8.0.2 requirements.
Also applies to: 441-441, 490-490
240-256: LGTM! Required Pods resources build phase added.The [CP] Copy Pods Resources build phase has been added to handle resource files from CocoaPods dependencies properly.
382-385: LGTM! Search paths format updated.The LD_RUNPATH_SEARCH_PATHS has been updated to use the modern array format across all configurations.
Also applies to: 509-512, 531-534
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2711 +/- ##
=================================================
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. |
|
@noman2002 please review. |
|
@palisadoes Please review. |
palisadoes
added
the
ignore-sensitive-files-pr
|
Please make a minor commit to trigger the workflows again. @noman2002 will be reviewing this shortly |
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?
Yes
Tests are written for all changes made in this PR.
Test coverage meets or exceeds the current coverage (~90/95%).
Snapshots/Videos:
Summary
Does this PR introduce a breaking change?
Checklist for Repository Standards
coderaabbitaireview suggestions?Other information
Have you read the contributing guide?
Summary by CodeRabbit
Platform Support
Image Handling
App Configuration
Testing Enhancements