Feature/share #2858
Feature/share #2858
Conversation
📝 WalkthroughWalkthroughAndroid build config was simplified by ignoring secrets.properties, commenting out Maps API key logic and product flavors, updating the manifest placeholder, and removing settings.gradle.kts. Flutter app adds a share action in AnalyticsSpecifics, introduces new imports, and updates pubspec with share/image/path_provider dependencies and a new logo asset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Widget as AnalyticsSpecifics
participant FS as Temp File System
participant Assets as Flutter Assets
participant Share as Share Sheet
User->>Widget: Tap share icon
activate Widget
Widget->>Assets: Load AirQo logo (PNG)
Assets-->>Widget: Byte data (logo)
Widget->>FS: Write logo to temp file
FS-->>Widget: File path
Widget->>Share: Share.shareXFiles(text, subject, file)
deactivate Widget
Note over Share,User: System presents native share UI (new interaction)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ 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 (
|
|
@Baalmart @Mozart299 , please review my pr, thanks |
![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 (8)
src/mobile-v3/android/app/src/main/AndroidManifest.xml (1)
11-13: Align comment with Secrets Gradle Plugin usage; placeholder change looks goodYou’ve switched the value to ${MAPS_API_KEY}, which is what the Secrets Gradle Plugin expects. The TODO above is now misleading—keys come from secrets.properties, not inline edits.
Apply this edit to clarify:
- <!-- TODO: Add your Google Maps API key here --> + <!-- Google Maps API key is provided by the Secrets Gradle Plugin. + Define MAPS_API_KEY in android/secrets.properties (ignored by VCS). -->src/mobile-v3/pubspec.yaml (2)
23-23: Trim trailing spaces to satisfy linters/CIYAMLlint flagged trailing spaces on these lines.
- + - path_provider: '^2.1.1' + path_provider: '^2.1.1'Also applies to: 36-36
35-37: Sanity-check new dependencies and surface area
- share_plus and path_provider versions look reasonable. No immediate conflicts with current minSdk (21) or targetSdk (35).
- The image package was added but isn’t used in analytics_specifics.dart (sharing uses rootBundle + File). If not used elsewhere, consider removing to reduce APK size.
Please run locally:
- flutter pub outdated
- rg -n "package:image" -g "!/build/" src/mobile-v3/lib
If unused, drop it:- image: ^4.1.3Also applies to: 82-82
src/mobile-v3/lib/src/app/dashboard/widgets/analytics_specifics.dart (2)
70-100: Harden share flow: better error handling, stable temp path, and optional richer messageCurrent flow works, but a few small tweaks will make it sturdier and a bit cleaner:
- Use a stable filename in the temp dir to avoid unbounded temp files on repeated shares.
- Prefer debugPrint (or your logger) over print; optionally surface a SnackBar on failure.
- Catch PlatformException explicitly (common from platform channels).
- Optional: if you have a canonical site link/deeplink, include siteId so the share feels personal.
Apply:
void _shareAirQualityData() async { - try { + try { final location = _getLocationDescription(widget.measurement); - //final locationId = widget.measurement.siteId ?? ''; - final deepLink = - 'https://airqo.net/products/mobile-app'; + final siteId = widget.measurement.siteDetails?.id; + // TODO: replace with a canonical deeplink if available, e.g., + // final deepLink = 'https://airqo.net/analytics?siteId=$siteId'; + final deepLink = 'https://airqo.net/products/mobile-app'; final shareText = ''' Check out the Air Quality of $location on the AirQo app! 👉 $deepLink '''; - // Load AirQo logo from assets - final byteData = await rootBundle.load('assets/images/airQo_logo.png'); + // Load AirQo logo from assets (registered in pubspec) + final byteData = await rootBundle.load('assets/images/airQo_logo.png'); - final tempDir = await getTemporaryDirectory(); - final logoFile = await File( - '${tempDir.path}/airqo_logo_${DateTime.now().millisecondsSinceEpoch}.png') - .writeAsBytes( - byteData.buffer.asUint8List(), - ); + final tempDir = await getTemporaryDirectory(); + final logoFile = File('${tempDir.path}/airqo_logo.png'); + if (!await logoFile.exists()) { + await logoFile.writeAsBytes(byteData.buffer.asUint8List()); + } // Share with logo and text await Share.shareXFiles( [XFile(logoFile.path)], text: shareText, subject: 'Air Quality Update from AirQo', ); - } catch (e) { - print("Error sharing data: $e"); + } on PlatformException catch (e) { + debugPrint('Share failed: ${e.message}'); + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Unable to share right now. Please try again.')), + ); + } catch (e, st) { + debugPrint('Error sharing data: $e\n$st'); } }
131-154: Improve accessibility and consider navigation regression from removing “close”
- Accessibility: wrap the tappable SVG with Tooltip/Semantics for screen readers.
- UX: the prior close action seems removed; verify there’s still an obvious way to dismiss this sheet/page.
Proposed tweak with a Tooltip:
- InkWell( + Tooltip(message: 'Share'), + InkWell( onTap: _shareAirQualityData, borderRadius: BorderRadius.circular(20), child: Container( padding: const EdgeInsets.all(8), child: SvgPicture.asset( "assets/icons/share-icon.svg",If a close/back affordance was intentionally removed, all good. If not, consider reintroducing it next to the share icon.
src/mobile-v3/android/app/build.gradle (3)
106-123: Product flavors commented out — confirm this won’t break distribution variantsWith flavors disabled, dev/prod separation (e.g., different appIdSuffix, keys) is gone. If that’s intentional, all good. If you still need variants, consider reintroducing minimal flavors that only carry manifestPlaceholders or appIdSuffix.
26-41: Dead code: properties loading blocks are now unusedappProperties and secretsProperties are loaded but never referenced after removing manual key wiring. You can drop these for a cleaner script.
-def appProperties = new Properties() -def appPropertiesFile = rootProject.file('key.properties') -if (appPropertiesFile.exists()) { - appPropertiesFile.withReader('UTF-8') { reader -> - appProperties.load(reader) - } -} - -def secretsProperties = new Properties() -def secretsPropertiesFile = rootProject.file('secrets.properties') -if (secretsPropertiesFile.exists()) { - secretsPropertiesFile.withReader('UTF-8') { reader -> - secretsProperties.load(reader) - } -} +// Removed unused appProperties and secretsProperties loaders (keys provided via Secrets Plugin)
50-59: Verify Secrets Plugin and MAPS_API_KEY ResolutionQuick validation shows:
- The
com.google.android.libraries.mapsplatform.secrets-gradle-pluginis applied inapp/build.gradle.- Your
AndroidManifest.xmlreferences the placeholder${MAPS_API_KEY}as expected.- A
settings.gradlefile exists undersrc/mobile-v3/android.Since you’ve removed the manual
googleMapApiKeylookups in favor of the Secrets Gradle Plugin, make sure that:
- Your CI pipelines and every developer’s machine include a
secrets.properties(orlocal.defaults.properties) definingMAPS_API_KEY(andMAPS_API_KEY_DEVif you use a separate dev key).- Without these files, the app will compile but crash or misconfigure maps at runtime—silent failures are hard to debug.
Optional: To fail fast on CI and catch missing keys early (without re-adding full flavor logic), you could inject a lightweight check in your Gradle configuration, for example in
app/build.gradlebefore yourandroid { … }block:// Fail build if MAPS_API_KEY isn’t defined by the Secrets plugin if (!project.hasProperty('MAPS_API_KEY')) { throw new GradleException( "Missing MAPS_API_KEY: add it to secrets.properties or local.defaults.properties" ) }This will immediately alert you in CI of a missing key without duplicating your old lookup logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
src/mobile-v3/assets/images/airQo_logo.pngis excluded by!**/*.pngsrc/mobile-v3/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
src/mobile-v3/android/.gitignore(1 hunks)src/mobile-v3/android/app/build.gradle(3 hunks)src/mobile-v3/android/app/src/main/AndroidManifest.xml(1 hunks)src/mobile-v3/android/settings.gradle.kts(0 hunks)src/mobile-v3/lib/src/app/dashboard/widgets/analytics_specifics.dart(3 hunks)src/mobile-v3/pubspec.yaml(4 hunks)
💤 Files with no reviewable changes (1)
- src/mobile-v3/android/settings.gradle.kts
🧰 Additional context used
🪛 YAMLlint (1.37.1)
src/mobile-v3/pubspec.yaml
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
src/mobile-v3/android/.gitignore (1)
8-8: No action needed—secrets.properties is already ignoredThe
git check-ignoreoutput confirms thatsrc/mobile-v3/android/secrets.propertiesis being ignored by the/android/secrets.propertiespattern insrc/mobile-v3/.gitignore(line 66), so there’s no risk of accidentally committing API keys. You can safely disregard the original suggestion to adjust the pattern insrc/mobile-v3/android/.gitignore.Likely an incorrect or invalid review comment.
src/mobile-v3/pubspec.yaml (1)
119-119: Asset registration matches usageassets/images/airQo_logo.png is correctly registered and matches the path used by the sharing code. LGTM.
src/mobile-v3/lib/src/app/dashboard/widgets/analytics_specifics.dart (1)
126-126: Color token changeChanging to AppColors.boldHeadlineColor4 is fine; just confirm this token meets contrast requirements against all supported themes/backgrounds.
|
@2phonebabykeem could you review |
2 participants
Added a share functionality for the analytics
Summary by CodeRabbit