fix: resolve silent document upload failures on Android 13+ by implem…#1059
Open
vibhutomer wants to merge 1 commit into
Open
fix: resolve silent document upload failures on Android 13+ by implem…#1059vibhutomer wants to merge 1 commit into
vibhutomer wants to merge 1 commit into
Conversation
…enting granular media permissions Signed-off-by: vibhutomer <vibhutomer25@gmail.com>
WalkthroughThis PR updates the Android registration app to support Android 13+ scoped media permissions for document upload. The manifest declares new granular ChangesAndroid 13+ Media Permission Compatibility
Sequence DiagramsequenceDiagram
participant User
participant ScanButton
participant DeviceInfo
participant PermissionHandler
participant CustomScanner
User->>ScanButton: tap scan button
ScanButton->>DeviceInfo: query androidInfo.version.sdkInt
alt SDK >= 33 (Android 13+)
ScanButton->>PermissionHandler: request Permission.photos
else SDK < 33 (Android 12 and below)
ScanButton->>PermissionHandler: request Permission.storage
end
alt permission granted
PermissionHandler-->>ScanButton: isGranted
ScanButton->>CustomScanner: navigate
else permission denied
PermissionHandler-->>ScanButton: isDenied
ScanButton->>User: show error SnackBar
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
android/app/src/main/AndroidManifest.xml (1)
4-9: ⚡ Quick winAdjust comment placement for clarity.
The comment on line 9 ("Storage - Android 13+") appears below
WRITE_EXTERNAL_STORAGEand aboveMANAGE_EXTERNAL_STORAGE, but the actual Android 13+ permissions (READ_MEDIA_IMAGESandREAD_MEDIA_VIDEO) are declared at lines 6-7. Consider moving the comment to line 6 to improve readability.♻️ Suggested comment reorganization
<!-- Storage - older Android --> <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" android:maxSdkVersion="32"/> + <!-- Storage - Android 13+ --> <uses-permission android:name="android.permission.READ_MEDIA_IMAGES" /> <uses-permission android:name="android.permission.READ_MEDIA_VIDEO" /> <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="29"/> - <!-- Storage - Android 13+ --> <!-- Full storage access for documents/PDFs (all Android versions) --> <uses-permission android:name="android.permission.MANAGE_EXTERNAL_STORAGE" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/AndroidManifest.xml` around lines 4 - 9, Move the "Storage - Android 13+" comment so it sits immediately above the Android 13+ permissions (the uses-permission entries with android:name="android.permission.READ_MEDIA_IMAGES" and android:name="android.permission.READ_MEDIA_VIDEO") instead of above/after WRITE_EXTERNAL_STORAGE; update the comment placement in AndroidManifest.xml to group the older storage permissions (READ_EXTERNAL_STORAGE, WRITE_EXTERNAL_STORAGE) under the "Storage - older Android" comment and the READ_MEDIA_* entries under the "Storage - Android 13+" comment for clearer organization.lib/ui/process_ui/widgets/document_upload_control.dart (1)
565-589: ⚡ Quick winConsider handling permanently denied permissions.
When users permanently deny permissions (by selecting "Don't ask again"), the current implementation shows the same error SnackBar without guiding them to Settings. Consider detecting
isDeniedvsisPermanentlyDeniedand offering to open app settings when appropriate.💡 Example implementation
if (!isGranted) { if (!mounted) return; final photosStatus = Platform.isAndroid && (await DeviceInfoPlugin().androidInfo).version.sdkInt >= 33 ? await Permission.photos.status : await Permission.storage.status; if (photosStatus.isPermanentlyDenied) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: const Text('Storage permission is required. Please enable it in Settings.'), backgroundColor: Colors.red, action: SnackBarAction( label: 'Settings', textColor: Colors.white, onPressed: () => openAppSettings(), ), ), ); } else { ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Storage permission is required to select documents.'), backgroundColor: Colors.red, ), ); } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/process_ui/widgets/document_upload_control.dart` around lines 565 - 589, The permission failure branch in the document selection flow (the Platform.isAndroid / DeviceInfoPlugin check using Permission.photos and Permission.storage) currently only shows a generic SnackBar; update the code in the same method/block to re-check the Permission.status (use the same Permission.photos or Permission.storage chosen by SDK version) and test for isPermanentlyDenied versus isDenied, call openAppSettings() via a SnackBarAction when isPermanentlyDenied, and preserve the existing SnackBar for non-permanent denials; also ensure you check mounted before showing the SnackBar to avoid context issues (refer to Permission.photos, Permission.storage, photosStatus/storageStatus, openAppSettings, Platform.isAndroid, DeviceInfoPlugin, and ScaffoldMessenger.of(context).showSnackBar).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ui/process_ui/widgets/document_upload_control.dart`:
- Around line 567-568: Wrap the DeviceInfoPlugin().androidInfo call and the
surrounding platform/permission logic inside a try-catch to handle exceptions
from device info retrieval; in the catch block log the error (e.g., debugPrint
or process logger) and fall back to requesting legacy storage permission so the
flow still proceeds. Update the block around DeviceInfoPlugin/androidInfo (and
related permission requests like Permission.photos.request and
Permission.storage.request) to use the try-catch, ensure
_documentScanClickedAudit() still runs before the try, and set the isGranted
boolean based on the fallback storage permission when an exception occurs.
- Around line 565-589: Duplicate permission-request logic in the mobile and
responsive handlers should be extracted into a single helper on the
_DocumentUploadControlState; add a private method (e.g. Future<bool>
_requestStoragePermission()) that contains the Platform.isAndroid/device SDK
check and returns whether permission was granted, then replace both duplicated
blocks with a call to _documentScanClickedAudit(); final isGranted = await
_requestStoragePermission(); and keep the existing SnackBar/return behavior if
false, followed by the existing navigator/addDocument/getScannedDocuments flow
(references: _DocumentUploadControlState, _requestStoragePermission,
_documentScanClickedAudit, addDocument, getScannedDocuments).
- Around line 565-589: This code calls ScaffoldMessenger.of(context) after
several awaits; add a mounted check in the surrounding State method (the async
handler that contains this permission/request logic, e.g., the
onPressed/_pickDocument method) immediately after the awaited permission calls
and before using context—i.e., after the final await(s) insert "if (!mounted)
return;" so the method exits if the widget was disposed, preventing use of
context when unmounted; ensure the check is placed before the
ScaffoldMessenger.of(context).showSnackBar(...) call and any other context
usage.
---
Nitpick comments:
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 4-9: Move the "Storage - Android 13+" comment so it sits
immediately above the Android 13+ permissions (the uses-permission entries with
android:name="android.permission.READ_MEDIA_IMAGES" and
android:name="android.permission.READ_MEDIA_VIDEO") instead of above/after
WRITE_EXTERNAL_STORAGE; update the comment placement in AndroidManifest.xml to
group the older storage permissions (READ_EXTERNAL_STORAGE,
WRITE_EXTERNAL_STORAGE) under the "Storage - older Android" comment and the
READ_MEDIA_* entries under the "Storage - Android 13+" comment for clearer
organization.
In `@lib/ui/process_ui/widgets/document_upload_control.dart`:
- Around line 565-589: The permission failure branch in the document selection
flow (the Platform.isAndroid / DeviceInfoPlugin check using Permission.photos
and Permission.storage) currently only shows a generic SnackBar; update the code
in the same method/block to re-check the Permission.status (use the same
Permission.photos or Permission.storage chosen by SDK version) and test for
isPermanentlyDenied versus isDenied, call openAppSettings() via a SnackBarAction
when isPermanentlyDenied, and preserve the existing SnackBar for non-permanent
denials; also ensure you check mounted before showing the SnackBar to avoid
context issues (refer to Permission.photos, Permission.storage,
photosStatus/storageStatus, openAppSettings, Platform.isAndroid,
DeviceInfoPlugin, and ScaffoldMessenger.of(context).showSnackBar).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d59b914d-1230-47a3-9a6d-813ce3eb2ca5
📒 Files selected for processing (2)
android/app/src/main/AndroidManifest.xmllib/ui/process_ui/widgets/document_upload_control.dart
Comment on lines
+565
to
+589
| bool isGranted = false; | ||
| if (Platform.isAndroid) { | ||
| final androidInfo = await DeviceInfoPlugin().androidInfo; | ||
| if (androidInfo.version.sdkInt >= 33) { | ||
| // Android 13+: Granular media permission | ||
| final photosStatus = await Permission.photos.request(); | ||
| isGranted = photosStatus.isGranted; | ||
| } else { | ||
| // Android 12 and below: Legacy storage permission | ||
| final storageStatus = await Permission.storage.request(); | ||
| isGranted = storageStatus.isGranted; | ||
| } | ||
| } else { | ||
| final storageStatus = await Permission.storage.request(); | ||
| isGranted = storageStatus.isGranted; | ||
| } | ||
| if (!isGranted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Storage permission is required to select documents.'), | ||
| backgroundColor: Colors.red, | ||
| ), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract duplicate permission logic into a helper method.
The identical permission-checking logic appears in both mobile (lines 565-589) and responsive (lines 860-895) layout handlers, violating the DRY principle. Extract this into a reusable helper method to improve maintainability.
♻️ Proposed refactor to eliminate duplication
Add this helper method to the _DocumentUploadControlState class:
Future<bool> _requestStoragePermission() async {
if (Platform.isAndroid) {
final androidInfo = await DeviceInfoPlugin().androidInfo;
if (androidInfo.version.sdkInt >= 33) {
// Android 13+: Granular media permission
final photosStatus = await Permission.photos.request();
return photosStatus.isGranted;
} else {
// Android 12 and below: Legacy storage permission
final storageStatus = await Permission.storage.request();
return storageStatus.isGranted;
}
} else {
final storageStatus = await Permission.storage.request();
return storageStatus.isGranted;
}
}Then replace both occurrences with:
_documentScanClickedAudit();
final isGranted = await _requestStoragePermission();
if (!isGranted) {
ScaffoldMessenger.of(context).showSnackBar(
const SnackBar(
content: Text('Storage permission is required to select documents.'),
backgroundColor: Colors.red,
),
);
return;
}
var doc = await Navigator.push(
context,
MaterialPageRoute(
builder: (context) =>
CustomScanner(field: widget.field)),
);
await addDocument(doc, widget.field, referenceNumber);
await getScannedDocuments(widget.field);Also applies to: 860-895
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ui/process_ui/widgets/document_upload_control.dart` around lines 565 -
589, Duplicate permission-request logic in the mobile and responsive handlers
should be extracted into a single helper on the _DocumentUploadControlState; add
a private method (e.g. Future<bool> _requestStoragePermission()) that contains
the Platform.isAndroid/device SDK check and returns whether permission was
granted, then replace both duplicated blocks with a call to
_documentScanClickedAudit(); final isGranted = await
_requestStoragePermission(); and keep the existing SnackBar/return behavior if
false, followed by the existing navigator/addDocument/getScannedDocuments flow
(references: _DocumentUploadControlState, _requestStoragePermission,
_documentScanClickedAudit, addDocument, getScannedDocuments).
Add mounted check before using context after async operations.
The code uses context in ScaffoldMessenger.of(context) (line 582) after multiple await calls without verifying the widget is still mounted. This can cause exceptions if the widget is disposed during the async operations.
🛡️ Proposed fix to add mounted check
if (!isGranted) {
+ if (!mounted) return;
ScaffoldMessenger.of(context).showSnackBar(
const SnackBar(
content: Text('Storage permission is required to select documents.'),
backgroundColor: Colors.red,
),
);
return;
}
+ if (!mounted) return;
var doc = await Navigator.push(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ui/process_ui/widgets/document_upload_control.dart` around lines 565 -
589, This code calls ScaffoldMessenger.of(context) after several awaits; add a
mounted check in the surrounding State method (the async handler that contains
this permission/request logic, e.g., the onPressed/_pickDocument method)
immediately after the awaited permission calls and before using context—i.e.,
after the final await(s) insert "if (!mounted) return;" so the method exits if
the widget was disposed, preventing use of context when unmounted; ensure the
check is placed before the ScaffoldMessenger.of(context).showSnackBar(...) call
and any other context usage.
Comment on lines
+567
to
+568
| final androidInfo = await DeviceInfoPlugin().androidInfo; | ||
| if (androidInfo.version.sdkInt >= 33) { |
There was a problem hiding this comment.
Add error handling for device info retrieval.
The call to DeviceInfoPlugin().androidInfo can throw an exception (e.g., on unsupported platforms or plugin initialization failures), but there's no try-catch block. Consider adding error handling to gracefully fallback or show a user-friendly message.
🛡️ Proposed fix with error handling
_documentScanClickedAudit();
bool isGranted = false;
try {
if (Platform.isAndroid) {
final androidInfo = await DeviceInfoPlugin().androidInfo;
if (androidInfo.version.sdkInt >= 33) {
final photosStatus = await Permission.photos.request();
isGranted = photosStatus.isGranted;
} else {
final storageStatus = await Permission.storage.request();
isGranted = storageStatus.isGranted;
}
} else {
final storageStatus = await Permission.storage.request();
isGranted = storageStatus.isGranted;
}
} catch (e) {
debugPrint("Error checking device info or permissions: $e");
// Fallback to legacy permission on error
final storageStatus = await Permission.storage.request();
isGranted = storageStatus.isGranted;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ui/process_ui/widgets/document_upload_control.dart` around lines 567 -
568, Wrap the DeviceInfoPlugin().androidInfo call and the surrounding
platform/permission logic inside a try-catch to handle exceptions from device
info retrieval; in the catch block log the error (e.g., debugPrint or process
logger) and fall back to requesting legacy storage permission so the flow still
proceeds. Update the block around DeviceInfoPlugin/androidInfo (and related
permission requests like Permission.photos.request and
Permission.storage.request) to use the try-catch, ensure
_documentScanClickedAudit() still runs before the try, and set the isGranted
boolean based on the fallback storage permission when an exception occurs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR resolves a critical device-fragmentation bug where the document upload feature silently failed on modern Android tablets (Android 13 / API 33+).
Previously, the application exclusively requested
Permission.storage(READ_EXTERNAL_STORAGE) before opening the file picker. On Android 13+, this legacy permission is completely deprecated and is silently denied by the OS without prompting the user, entirely blocking the document registration workflow.Changes Made
READ_MEDIA_IMAGESandREAD_MEDIA_VIDEO) to support modern Android API levels.device_info_plusto conditionally check the Android SDK version at runtime. The app now correctly requestsPermission.photosfor Android 13+ devices, while maintaining legacyPermission.storagesupport for older devices.Related Issue
Closes #1058
Type of Change
Checklist
Summary by CodeRabbit