Enhance Simulator Download & External Link Handling with Android 13+ Media Support #421
Conversation
WalkthroughAdds Android storage and Android 13 media permissions, two Android MethodChannels for media scanning and MediaStore saving, converts SimulatorView to a StatefulWidget with in-webview back navigation and external URL launching, and refactors SimulatorViewModel download flow, storage-permission checks, file saving, and extension/mime detection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 8
🔭 Outside diff range comments (1)
android/app/src/main/AndroidManifest.xml (1)
46-46: requestLegacyExternalStorage is ignored on targetSdk ≥ 30; remove itOn Android 11+, this flag has no effect. Rely on MediaStore or app-specific directories instead.
- android:requestLegacyExternalStorage="true"If you retain it for older build variants, consider:
- android:requestLegacyExternalStorage="true" + android:requestLegacyExternalStorage="true" + tools:remove="android:requestLegacyExternalStorage"…and apply it only in a legacy manifest or flavor.
🧹 Nitpick comments (9)
android/app/src/main/AndroidManifest.xml (1)
2-2: tools namespace added but unused; consider using it or removingYou can use tools to remove deprecated attributes (e.g., requestLegacyExternalStorage) during manifest merge. Otherwise, drop the namespace to avoid lint noise.
-<manifest xmlns:android="http://schemas.android.com/apk/res/android" - xmlns:tools="http://schemas.android.com/tools"> +<manifest xmlns:android="http://schemas.android.com/apk/res/android">Or keep it and apply tools:remove as suggested below.
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt (2)
9-9: Align MethodChannel name with app package to avoid confusionUse a channel name under your actual package namespace.
- private val CHANNEL = "com.example.mobile_app/media_scanner" + private val CHANNEL = "org.circuitverse.mobile_app/media_scanner"Remember to update the Dart side accordingly where the channel is invoked.
31-47: Return a simple boolean and ensure result callbacks are marshalled to UI threadReturning a boolean makes the Dart side simpler. Also marshal all result callbacks.
- private fun scanFile(path: String, result: MethodChannel.Result) { + private fun scanFile(path: String, result: MethodChannel.Result) { try { MediaScannerConnection.scanFile( this, arrayOf(path), null ) { _, uri -> - if (uri != null) { - result.success("File scanned successfully") - } else { - result.error("SCAN_FAILED", "Failed to scan file", null) - } + runOnUiThread { + if (uri != null) { + result.success(true) + } else { + result.error("SCAN_FAILED", "Failed to scan file", null) + } + } } } catch (e: Exception) { - result.error("SCAN_ERROR", "Error scanning file: ${e.message}", null) + runOnUiThread { + result.error("SCAN_ERROR", "Error scanning file: ${e.message}", null) + } } }lib/ui/views/simulator/simulator_view.dart (2)
91-93: Optional: pass more context to downloads (filename/contentDisposition)If available, thread through contentDisposition/suggestedFilename so the ViewModel can derive better filenames.
- onDownloadStartRequest: (controller, request) { - model.download(request); - }, + onDownloadStartRequest: (controller, request) { + model.download(request); + },Note: DownloadStartRequest already contains mimeType, contentDisposition, and suggestedFilename; ensure the ViewModel uses these fields.
33-47: Ensure Flutter SDK supports PopScope.onPopInvokedWithResult or add onPopInvoked fallbackWe found multiple usages of
onPopInvokedWithResult. Verify your Flutter SDK version and, if it doesn’t include this API yet, add anonPopInvokedfallback with identical logic (minus theresultparameter) to avoid runtime errors.• lib/ui/views/simulator/simulator_view.dart (line 35)
• lib/ui/views/projects/project_details_view.dart (line 687)
• lib/ui/views/cv_landing_view.dart (line 109)
• lib/ui/views/ib/ib_landing_view.dart (line 321)Example fallback implementation:
PopScope( canPop: false, onPopInvoked: (didPop) async { if (didPop) return; if (webViewController != null) { final canGoBack = await webViewController!.canGoBack(); if (canGoBack) { await webViewController!.goBack(); return; } } Navigator.of(context).pop(); }, child: IndexedStack(...), )lib/viewmodels/simulator/simulator_viewmodel.dart (4)
114-121: Incorporate suggestedFilename/contentDisposition in filename generationUse a human-friendly name when provided by the server.
- final extension = _getFileExtension( - request.url.toString(), - request.mimeType, - ); - final fileName = - 'CircuitVerse_${DateTime.now().millisecondsSinceEpoch}$extension'; + final extension = _getFileExtension( + url: request.url.toString(), + mimeType: request.mimeType, + contentDisposition: request.contentDisposition, + suggestedFilename: request.suggestedFilename, + ); + final base = request.suggestedFilename ?? + 'CircuitVerse_${DateTime.now().millisecondsSinceEpoch}'; + final fileName = base.endsWith(extension) ? base : '$base$extension';
149-159: Success message can be misleading when fallback directory is usedIf saving to an app-specific directory, don’t claim “Pictures/CircuitVerse”. Use the actual directory path in the message.
- SnackBarUtils.showDark( - 'Download Complete!', - Platform.isAndroid - ? 'Saved to Pictures/CircuitVerse: $fileName' - : 'Saved to app directory: $fileName', - ); + final loc = '${downloadDir.path}/$fileName'; + SnackBarUtils.showDark('Download Complete!', 'Saved to: $loc');
197-212: Behavior drift: view now opens learn.circuitverse.org externally; ViewModel still routes to IbLandingViewfindMatchInString still handles learn.circuitverse.org by navigating to IbLandingView, but the View intercepts and opens externally. Unify the approach (either external or in-app) to prevent inconsistent UX and dead code.
175-177: Surface underlying error cause in snackbar without leaking sensitive dataConsider trimming long exception messages and including status code if available.
- SnackBarUtils.showDark('Download Failed', 'Error: ${e.toString()}'); + SnackBarUtils.showDark('Download Failed', 'Error: ${e.runtimeType}: ${e is Exception ? e.toString() : 'See logs'}');Also log the full error with stack trace via debugPrint for developer builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/app/src/main/AndroidManifest.xml(3 hunks)android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt(1 hunks)lib/ui/views/simulator/simulator_view.dart(2 hunks)lib/viewmodels/simulator/simulator_viewmodel.dart(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (4)
android/app/src/main/AndroidManifest.xml (1)
39-41: No changes needed for SDK versions regarding READ_MEDIA_ permissions*Your
android/app/build.gradleis already set to compileSdkVersion 35 and targetSdkVersion 33, which meets the Android 13 (SDK 33) requirement for theREAD_MEDIA_IMAGES,READ_MEDIA_VIDEO, andREAD_MEDIA_AUDIOpermissions.
Proceed with runtime permission handling as usual.lib/ui/views/simulator/simulator_view.dart (2)
8-8: Good additionurl_launcher import aligns with external link handling.
10-17: Stateful migration looks correctState management via controller is appropriate for back navigation.
lib/viewmodels/simulator/simulator_viewmodel.dart (1)
36-36: Minor: token getter simplification LGTM
Pull Request Test Coverage Report for Build 16911328686Details
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ui/views/simulator/simulator_view.dart (1)
78-101: Harden URL interception: handle non-http(s) schemes, subdomains, and give UX feedback on launch failures.Thanks for addressing the earlier feedback (URI parsing + launchUrl result handling). The following refinements align with your manifest queries (mailto/tel) and improve robustness.
shouldOverrideUrlLoading: (controller, action) async { - final uri = action.request.url; - if (uri == null) { - return NavigationActionPolicy.ALLOW; - } - - final urlStr = uri.toString(); - final host = uri.host.toLowerCase(); - final path = uri.path.toLowerCase(); - final isLearn = host == 'learn.circuitverse.org'; - final isPdf = path.endsWith('.pdf'); - - if (isLearn || isPdf) { - final ok = await launchUrl( - Uri.parse(urlStr), - mode: LaunchMode.externalApplication, - ); - if (ok) { - return NavigationActionPolicy.CANCEL; - } - } - - return NavigationActionPolicy.ALLOW; + final uri = action.request.url; + if (uri == null) { + return NavigationActionPolicy.ALLOW; + } + final scheme = uri.scheme.toLowerCase(); + + // 1) Handle non-http(s) schemes (e.g., mailto:, tel:, intent:, market:) + if (scheme != 'http' && scheme != 'https') { + final ok = await launchUrl( + uri, + mode: LaunchMode.externalApplication, + ); + return ok + ? NavigationActionPolicy.CANCEL + : NavigationActionPolicy.ALLOW; + } + + // 2) Handle specific external domains and file types + final host = uri.host.toLowerCase(); + final path = uri.path.toLowerCase(); + final isLearn = host == 'learn.circuitverse.org' || host.endsWith('.learn.circuitverse.org'); + final isPdf = path.endsWith('.pdf'); + + if (isLearn || isPdf) { + final ok = await launchUrl( + uri, + mode: LaunchMode.externalApplication, + ); + if (ok) { + return NavigationActionPolicy.CANCEL; + } + // Notify users if we couldn’t open externally, then allow WebView to try. + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Could not open externally. Loading in-app…')), + ); + } + return NavigationActionPolicy.ALLOW; },Optional: handle target="_blank" windows so they open externally instead of silently failing in-WebView.
shouldOverrideUrlLoading: (controller, action) async { ... }, + onCreateWindow: (controller, createWindowAction) async { + final uri = createWindowAction.request.url; + if (uri != null) { + final ok = await launchUrl( + uri, + mode: LaunchMode.externalApplication, + ); + // true = we handled it; false = let WebView attempt it + return ok; + } + return false; + },Test suggestion: add unit/widget tests for:
- mailto:/tel: intercepted and launchUrl invoked.
- PDFs and learn.circuitverse.org links opened externally; failure falls back to ALLOW.
You can locate/add tests with:
#!/bin/bash # Find tests referencing SimulatorView or shouldOverrideUrlLoading rg -n "SimulatorView|shouldOverrideUrlLoading|InAppWebView" test || true
🧹 Nitpick comments (3)
lib/ui/views/simulator/simulator_view.dart (3)
33-47: Streamline back-navigation check and add a small readability polish.Use a null-aware pattern to reduce nesting.
- if (webViewController != null) { - bool canGoBack = await webViewController!.canGoBack(); - if (canGoBack) { - await webViewController!.goBack(); - return; - } - } + if (await (webViewController?.canGoBack() ?? Future.value(false))) { + await webViewController!.goBack(); + return; + }Also consider adding a widget test to assert:
- When WebView can go back, PopScope prevents route pop and calls goBack.
- When it cannot, route pop occurs.
I can scaffold a widget test harness with a fake controller to verify this behavior.
50-56: Improve accessibility of the progress indicator.Add semanticsLabel so screen readers announce loading state.
child: CircularProgressIndicator( backgroundColor: Colors.grey[300], color: Colors.grey[500], strokeWidth: 3.0, + semanticsLabel: 'Loading simulator…', ),
69-76: Initial URL/settings look correct; consider multi-window support.Target="_blank" links won’t open without supportMultipleWindows and an onCreateWindow handler.
initialSettings: InAppWebViewSettings( useShouldOverrideUrlLoading: true, useOnDownloadStart: true, + supportMultipleWindows: true, + javaScriptCanOpenWindowsAutomatically: true, ),You can then handle new windows via onCreateWindow (see comment below).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/app/src/main/AndroidManifest.xml(3 hunks)android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt(1 hunks)lib/ui/views/simulator/simulator_view.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt
- android/app/src/main/AndroidManifest.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (4)
lib/ui/views/simulator/simulator_view.dart (4)
10-17: Good migration to StatefulWidget.This enables back navigation handling and controller lifecycle. LGTM.
19-21: Controller storage looks fine.Nullable controller with late assignment is appropriate here.
101-103: Download delegation looks good.Assuming ViewModel handles permissions, data URLs, auth headers, and error UX.
If not already covered, add a snackbar/toast on success/failure in the ViewModel so users get feedback after tapping a download.
69-72: Token header usage check.If token can be null/expired, consider a guard path (redirect to login or show an auth error) before attempting to load the simulator.
I can wire a minimal auth-failure handler (e.g., onReceivedHttpError/onLoadHttpError) to catch 401/403 and prompt re-auth.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt (1)
17-33: Wrap all MethodChannel result callbacks on UI thread (regression from earlier fix)Result methods are still called off the UI thread. This was flagged previously and marked addressed, but the current code still calls result.success/error/notImplemented directly, including inside MediaScanner callback. Wrap them with runOnUiThread for both channels.
- MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_scanner") - .setMethodCallHandler { call, result -> + MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_scanner") + .setMethodCallHandler { call, result -> if (call.method == "scanFile") { val path = call.argument<String>("path") if (path != null) { MediaScannerConnection.scanFile(this, arrayOf(path), null) { _, uri -> - if (uri != null) result.success("File scanned successfully") - else result.error("SCAN_FAILED", "Failed to scan file", null) + runOnUiThread { + if (uri != null) result.success("File scanned successfully") + else result.error("SCAN_FAILED", "Failed to scan file", null) + } } } else { - result.error("INVALID_ARGUMENT", "Path cannot be null", null) + runOnUiThread { result.error("INVALID_ARGUMENT", "Path cannot be null", null) } } } else { - result.notImplemented() + runOnUiThread { result.notImplemented() } } }And similarly for the media_store channel:
- "getApiLevel" -> result.success(Build.VERSION.SDK_INT) + "getApiLevel" -> runOnUiThread { result.success(Build.VERSION.SDK_INT) } - else -> result.notImplemented() + else -> runOnUiThread { result.notImplemented() }
🧹 Nitpick comments (4)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt (2)
35-36: Namespace the MethodChannel with your package (consistency)Use the app package as the channel prefix (org.circuitverse.mobile_app) instead of com.example to avoid collisions and for clarity. Mirror the change in Dart constants.
-MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_scanner") +MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "org.circuitverse.mobile_app/media_scanner") ... -MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_store") +MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "org.circuitverse.mobile_app/media_store")Also applies to: 17-18
55-76: Optional: return saved Uri instead of booleanReturning the content Uri (String) enables better UX (e.g., share/open). Keep backward compatibility by accepting both and using boolean as a fallback.
- runOnUiThread { result.success(true) } + runOnUiThread { result.success(it.toString()) }And update Dart to accept a String (Uri) or bool.
lib/viewmodels/simulator/simulator_viewmodel.dart (2)
297-305: Skip media scan for app-specific paths on API 29+ (it won’t surface in gallery)MediaScanner won’t surface files saved under app-specific directories on scoped storage. Guard scan by API level and/or path.
- if (Platform.isAndroid && _isImageFile(extension)) { + if (Platform.isAndroid && _isImageFile(extension)) { try { - await _mediaScannerChannel.invokeMethod('scanFile', { - 'path': file.path, - }); + final apiLevel = await _mediaStoreChannel.invokeMethod<int>('getApiLevel') ?? 28; + if (apiLevel <= 28) { + await _mediaScannerChannel.invokeMethod('scanFile', { + 'path': file.path, + }); + } } catch (e) { debugPrint('Media scan error: $e'); } }
184-207: Follow-up: Unit tests needed for regressions (coverage dropped by 0.3%)Add tests for:
- _getFileExtension parsing (suggestedFilename, contentDisposition, URL, MIME fallbacks)
- _getMimeType defaults
- Redirect handling loop (limit and location resolution)
- Cookie attachment uses current URL
I can scaffold these with fake HttpClient and mocked CookieManager.
Would you like me to open a PR adding these tests to recover coverage?
Also applies to: 233-242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt(1 hunks)lib/viewmodels/simulator/simulator_viewmodel.dart(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt (2)
17-32: Ensure MethodChannel results are posted on the UI thread (still missing for media_scanner handler).Result callbacks in the scanFile path are invoked from a background thread; post them to the main thread to avoid threading issues. Similar feedback was given earlier and appears only partially applied.
Apply this diff:
MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_scanner") .setMethodCallHandler { call, result -> if (call.method == "scanFile") { val path = call.argument<String>("path") if (path != null) { MediaScannerConnection.scanFile(this, arrayOf(path), null) { _, uri -> - if (uri != null) result.success("File scanned successfully") - else result.error("SCAN_FAILED", "Failed to scan file", null) + runOnUiThread { + if (uri != null) result.success("File scanned successfully") + else result.error("SCAN_FAILED", "Failed to scan file", null) + } } } else { - result.error("INVALID_ARGUMENT", "Path cannot be null", null) + runOnUiThread { + result.error("INVALID_ARGUMENT", "Path cannot be null", null) + } } } else { - result.notImplemented() + runOnUiThread { result.notImplemented() } } }
34-52: Wrap media_store MethodChannel result calls with runOnUiThread for consistency and safety.Even if handler runs on the platform thread, posting to UI thread is a safer default and matches the rest of your codebase.
MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_store") .setMethodCallHandler { call, result -> when (call.method) { - "getApiLevel" -> result.success(Build.VERSION.SDK_INT) + "getApiLevel" -> runOnUiThread { result.success(Build.VERSION.SDK_INT) } "saveToPictures" -> { val bytes = call.argument<ByteArray>("bytes") val filename = call.argument<String>("filename") val mimeType = call.argument<String>("mimeType") if (bytes != null && filename != null && mimeType != null) { saveToPictures(bytes, filename, mimeType, result) } else { - result.error("INVALID_ARGUMENT", "Missing required arguments", null) + runOnUiThread { + result.error("INVALID_ARGUMENT", "Missing required arguments", null) + } } } - else -> result.notImplemented() + else -> runOnUiThread { result.notImplemented() } } }
🧹 Nitpick comments (4)
lib/viewmodels/simulator/simulator_viewmodel.dart (4)
26-33: Use your real applicationId in MethodChannel names to avoid collisions (avoid com.example.*).Prefer a reverse-DNS based on your package (org.circuitverse.mobile_app) instead of com.example to prevent clashes across plugins/apps.
- static const _mediaScannerChannel = MethodChannel( - 'com.example.mobile_app/media_scanner', - ); + static const _mediaScannerChannel = MethodChannel( + 'org.circuitverse.mobile_app/media_scanner', + ); - static const _mediaStoreChannel = MethodChannel( - 'com.example.mobile_app/media_store', - ); + static const _mediaStoreChannel = MethodChannel( + 'org.circuitverse.mobile_app/media_store', + );Apply the same change in MainActivity.kt:
-MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_scanner") +MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "org.circuitverse.mobile_app/media_scanner") -MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "com.example.mobile_app/media_store") +MethodChannel(flutterEngine.dartExecutor.binaryMessenger, "org.circuitverse.mobile_app/media_store")
53-89: Permission flow: handle permanently denied case for Android ≤ 28.If the user permanently denies storage, offer to open app settings; otherwise, UX gets stuck.
final permission = Permission.storage; if (await permission.isGranted) return true; - final status = await permission.request(); + final status = await permission.request(); debugPrint('Storage permission status: $status'); + if (status.isPermanentlyDenied) { + // Optionally inform user and navigate to settings. + await openAppSettings(); + return false; + } return status.isGranted;
107-147: Filename parsing: good coverage of suggestedFilename and Content-Disposition. Consider sanitization.Edge-case: malicious filenames like "../../../../foo.png". While you only extract the extension, be sure later filename usage doesn’t include untrusted path segments. Current logic is safe but keep this in mind if you start using the full filename.
195-225: Harden HTTP client with timeouts to avoid hangs.Long network stalls will block the UI feedback path. Add connection/idle timeouts.
- final httpClient = HttpClient(); + final httpClient = HttpClient() + ..connectionTimeout = const Duration(seconds: 20)) + ..idleTimeout = const Duration(seconds: 15);Optionally cap payload size if you expect small files to avoid memory spikes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt(1 hunks)lib/viewmodels/simulator/simulator_viewmodel.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
android/app/src/main/kotlin/org/circuitverse/mobile_app/MainActivity.kt (1)
55-86: MediaStore save implementation is robust.Good handling of null OutputStream, IS_PENDING, and posting results on the UI thread. This should work reliably on API 29+.
lib/viewmodels/simulator/simulator_viewmodel.dart (2)
149-160: MIME defaults are safe.Defaulting to application/octet-stream is correct and avoids misclassifying arbitrary content as images.
162-166: Confirm intent: SVGs are excluded from “image” handling.You map .svg to image/svg+xml but exclude it from _isImageFile. If you want SVGs saved via MediaStore Images, add '.svg'; if not, leave as-is.
Do you want me to add .svg to the image set?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/viewmodels/simulator/simulator_viewmodel.dart(4 hunks)
🔇 Additional comments (11)
lib/viewmodels/simulator/simulator_viewmodel.dart (11)
26-32: LGTM! Method channels properly configured.The MethodChannel declarations are correctly configured with the appropriate channel names that match the Android implementation in MainActivity.kt.
44-44: LGTM! Simplified token getter.The simplified token getter correctly returns the token when logged in and null otherwise.
53-89: LGTM! Platform-specific permission handling correctly implemented.The permission logic correctly:
- Skips permissions on iOS (returns true)
- Uses MediaStore channel to get API level on Android
- Requires no permissions for API 29+ (MediaStore handles writing)
- Requests storage permission only for API 28 and below
- Includes proper fallback handling for errors
This aligns with Android's scoped storage requirements and the previous review feedback.
91-105: LGTM! Safe directory resolution for all platforms.The download directory logic correctly:
- Uses app-specific external storage on Android (safe for all API levels)
- Falls back to app documents directory on iOS or when external storage is unavailable
- Creates the Downloads subdirectory if it doesn't exist
This approach avoids direct writes to public directories that would fail on Android 10+.
107-147: LGTM! Comprehensive file extension detection.The
_getFileExtensionmethod correctly implements the priority order:
- Suggested filename from WebView
- Content-Disposition header parsing (RFC 6266 compliant)
- URL path extraction
- MIME type mapping
- Safe fallback to
.binThe regex pattern correctly handles both
filename=andfilename*=variants with proper URI decoding.
149-165: LGTM! Proper MIME type mapping and image detection.Both helper methods correctly:
- Map common file extensions to appropriate MIME types
- Use safe defaults (
application/octet-streamfor unknown types)- Identify image files for special handling
The extension-to-MIME mapping is comprehensive and uses standard MIME types.
167-187: LGTM! Robust download initialization.The download method properly:
- Checks storage permissions first
- Shows clear error messages for permission denial
- Generates unique filenames with timestamps
- Uses the comprehensive extension detection method
The filename pattern
CircuitVerse_<timestamp><extension>ensures uniqueness and clear identification.
190-225: LGTM! Comprehensive download handling with proper redirect support.The download logic correctly handles:
- Data URLs by extracting bytes from the data URI
- HTTP URLs with proper redirect following (up to 5 redirects)
- Authorization headers when user is logged in
- Cookies from the WebView's CookieManager for the current URL
- Proper cleanup with
httpClient.close()in finally blockThe redirect handling correctly updates the
currentURL and drains response bodies before continuing.
227-288: LGTM! Platform-aware file saving with proper MediaStore integration.The file saving logic correctly:
- Validates that bytes were received
- Uses MediaStore for Android 10+ images via the platform channel
- Passes
Uint8List.fromList(bytes)to ensure proper byte array transfer- Falls back to filesystem writes for non-images or older Android versions
- Triggers media scanner only for legacy Android image saves
- Shows appropriate success messages with actual file paths
The error handling ensures graceful fallback from MediaStore to filesystem when needed.
285-287: LGTM! Proper error handling with user feedback.The catch block correctly:
- Shows user-friendly error messages via SnackBar
- Prevents exceptions from crashing the UI
- Includes the actual error message for debugging
306-325: LGTM! Clear URL pattern matching with helpful comments.The URL matching logic is well-documented with clear comments explaining each pattern check:
- Project edit URLs
- Project save/update URLs
- Groups/users URLs
The comments improve code readability and maintainability.
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
3 participants
Fixes #422
Describe the changes you have made in this PR
AndroidManifest.xml
xmlns:toolsnamespace.READ_MEDIA_IMAGESREAD_MEDIA_VIDEOREAD_MEDIA_AUDIOREAD_EXTERNAL_STORAGEWRITE_EXTERNAL_STORAGEqueriesformailto,https, andtelschemes.SimulatorView
StatelessWidgettoStatefulWidgetfor back navigation handling viaPopScope.url_launcherto open external links (e.g., user manual PDFs) outside the WebView.SimulatorViewModel
Permission.storage,Permission.photos).Pictures/CircuitVerseon Android for better gallery visibility.MainActivity.kt
"com.example.mobile_app/media_scanner"to allow Flutter to invoke Android’sMediaScannerConnectionfor newly downloaded files.scanFilemethod to refresh the media library after downloads, ensuring images appear in the gallery immediately.Screenshots of the changes (If any)
Note: Please check Allow edits from maintainers so we can help refine the PR if needed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes