Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device Settings, Intercom Integration and Improvements #902

Merged
merged 14 commits into from
Sep 24, 2024
Merged

Conversation

mdmohsin7
Copy link
Collaborator

@mdmohsin7 mdmohsin7 commented Sep 21, 2024

  • Move device settings to settings page
  • Move getDeviceInfo to device provider
  • Guides and support should be highlighted further
  • Improve the guides UI (dialogs look bad)
  • An individual item on device settings, about how to charge it, opens a separate page with images and clear instructions (using intercom)
  • This page above should also be accessible from device details page below the battery indicator (using intercom)
  • Migrate AGP to 8+ and Proguard changes
  • Integrate intercom (with push notifications)
  • Add need help? chat with us option at the top in settings
ScreenRecording_09-23-2024.12-20-12_1.1.mp4

Summary by Entelligence.AI

  • New Feature: Introduced a separate page for device settings, enhancing the UI and navigation flow.
  • New Feature: Added a button to access specific device information and a text button for navigating to device charging instructions.
  • Refactor: Moved device settings related functions to DeviceSettings widget, improving code modularity.
  • New Feature: Enhanced SupportPage with ExpansionTileCard for better presentation of firmware updates and bootloader support information. Added links to guides for manual firmware and bootloader updates.
  • Chore: Updated Gradle distribution URL to version 8.0 and iOS platform version to 15.0 for improved compatibility and feature availability.
  • New Feature: Integrated Intercom SDK for chat support and push notifications, providing a better user experience.
  • Refactor: Added environment variables for Intercom configuration, improving code maintainability and security.

Copy link

github-actions bot commented Sep 21, 2024

Image description Entelligence.AI

Walkthrough

This pull request introduces significant changes to the Flutter app's UI, navigation flow, and device settings management. It also updates the Android and iOS build configurations, integrates Intercom for chat support and push notifications, and adds environment variables for Intercom configuration.

Changes

Files Summary
app/lib/pages/capture/connect.dart,
app/lib/pages/home/device.dart,
app/lib/pages/settings/page.dart,
app/lib/pages/home/device_settings.dart,
app/lib/pages/settings/device_settings.dart,
app/lib/pages/home/support.dart,
app/lib/pages/settings/widgets/expansion_tile_card.dart,
app/lib/main.dart,
app/lib/providers/device_provider.dart
These changes revamp the UI and navigation flow of the Flutter app, moving device settings to a separate page, adding new features related to device settings and interactions, and introducing an expandable card widget for displaying support information. The DeviceProvider class is updated with a new method for fetching device information.
app/android/app/build.gradle,
app/android/app/proguard-rules.pro,
app/android/build.gradle,
app/android/gradle/wrapper/gradle-wrapper.properties,
app/android/settings.gradle,
app/ios/Podfile,
app/ios/Runner.xcodeproj/project.pbxproj,
app/ios/Runner/Info.plist
These diffs update the Android and iOS build configurations, including Proguard settings, Gradle distribution URL, Android namespace, iOS platform version, and camera access requirement in the Info.plist file.
app/lib/env/dev_env.dart,
app/lib/env/env.dart,
app/lib/env/prod_env.dart,
app/android/app/build.gradle,
app/android/app/proguard-rules.pro,
app/android/app/src/main/AndroidManifest.xml,
app/android/app/src/main/java/com/friend/ios/MyApp.kt,
app/lib/pages/settings/page.dart,
app/lib/services/notification_service.dart
These changes integrate Intercom for chat support and push notifications, adding Intercom SDK dependencies, initializing the SDK in an Android application, and sending FCM token to Intercom. Environment variables for Intercom configuration are added in the DevEnv and ProdEnv classes.

"In the land of code where logic is king, 🤴

Changes were made, new features they bring. 🎁

UI revamped, settings have a new ring, 🔔

Intercom chats, messages they'll fling. 💬

Build configs updated, like a bird on the wing, 🐦

Celebrate these changes, let us sing! 🎶"


Uplevel your code reviews with Entelligence.AI Pro

Entelligence.AI Pro

If you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between 5f21590 and 4331261 commits.
Files selected (8)
  • app/lib/pages/capture/connect.dart (2)
  • app/lib/pages/home/device.dart (3)
  • app/lib/pages/home/device_settings.dart (1)
  • app/lib/pages/home/support.dart (2)
  • app/lib/pages/settings/device_settings.dart (1)
  • app/lib/pages/settings/page.dart (2)
  • app/lib/pages/settings/widgets/expansion_tile_card.dart (1)
  • app/lib/providers/device_provider.dart (2)
Files skipped from review due to trivial changes (2)
  • app/lib/pages/home/support.dart
  • app/lib/pages/settings/page.dart
Review comments generated (12)
  • Review: 9
  • LGTM: 3

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

@@ -1,5 +1,5 @@
import 'package:flutter/material.dart';
import 'package:friend_private/pages/home/device_settings.dart';
import 'package:friend_private/pages/settings/device_settings.dart';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The import path for DeviceSettings has been updated. Ensure that the new path 'package:friend_private/pages/settings/device_settings.dart' is correct and the file exists at this location.

- import 'package:friend_private/pages/home/device_settings.dart';
+ import 'package:friend_private/pages/settings/device_settings.dart';

builder: (context) => const DeviceSettings(
isDeviceConnected: false,
),
builder: (context) => const DeviceSettings(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The instantiation of DeviceSettings no longer includes the isDeviceConnected parameter. If this parameter was used to control any logic within DeviceSettings, ensure that its removal doesn't cause any unintended behavior.

- builder: (context) => const DeviceSettings(isDeviceConnected: false),
+ builder: (context) => const DeviceSettings(),

Comment on lines 163 to 175
const SizedBox(height: 8),
TextButton(
onPressed: () {
routeToPage(context, const DeviceSettings());
},
child: const Text(
'How to charge?',
style: TextStyle(
color: Colors.white,
decoration: TextDecoration.underline,
),
),
),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

A new TextButton has been added to navigate to the DeviceSettings page when clicked. However, it seems like there's no parameter passed to DeviceSettings(). If DeviceSettings requires any mandatory parameters, this will cause an error. Please ensure that all required parameters are passed correctly.

- routeToPage(context, const DeviceSettings());
+ routeToPage(context, DeviceSettings(device: widget.device));

Comment on lines +24 to +31
// TODO: thinh, use connection directly
Future _bleDisconnectDevice(BTDeviceStruct btDevice) async {
var connection = await ServiceManager.instance().device.ensureConnection(btDevice.id);
if (connection == null) {
return Future.value(null);
}
return await connection.disconnect();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The _bleDisconnectDevice method is using a direct connection to disconnect the device. This could lead to issues if the connection is not properly managed or if there are multiple connections to the same device. It would be better to use a centralized service for managing device connections, which can ensure that connections are properly handled and prevent potential issues with multiple connections.

-  Future _bleDisconnectDevice(BTDeviceStruct btDevice) async {
-    var connection = await ServiceManager.instance().device.ensureConnection(btDevice.id);
-    if (connection == null) {
-      return Future.value(null);
-    }
-    return await connection.disconnect();
-  }
+  Future _bleDisconnectDevice(BTDeviceStruct btDevice) async {
+    await ServiceManager.instance().device.disconnectDevice(btDevice.id);
+  }

Comment on lines +130 to +147
onPressed: () async {
await SharedPreferencesUtil().btDeviceStructSet(BTDeviceStruct(id: '', name: ''));
SharedPreferencesUtil().deviceName = '';
if (provider.connectedDevice != null) {
await _bleDisconnectDevice(provider.connectedDevice!);
}
provider.setIsConnected(false);
provider.setConnectedDevice(null);
provider.updateConnectingStatus(false);
context.read<OnboardingProvider>().stopFindDeviceTimer();
Navigator.of(context).pop();
Navigator.of(context).pop();
ScaffoldMessenger.of(context).showSnackBar(SnackBar(
content: Text(
'Your Friend is ${provider.connectedDevice == null ? "unpaired" : "disconnected"} 😔'),
));
MixpanelManager().disconnectFriendClicked();
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

This block of code seems to be doing a lot of operations related to disconnecting a device. It might be more maintainable and easier to understand if these operations were encapsulated into a single method within the DeviceProvider class. This way, you can just call that method here, making the code cleaner and easier to read.

- await SharedPreferencesUtil().btDeviceStructSet(BTDeviceStruct(id: '', name: ''));
- SharedPreferencesUtil().deviceName = '';
- if (provider.connectedDevice != null) {
-   await _bleDisconnectDevice(provider.connectedDevice!);
- }
- provider.setIsConnected(false);
- provider.setConnectedDevice(null);
- provider.updateConnectingStatus(false);
- context.read<OnboardingProvider>().stopFindDeviceTimer();
+ await provider.disconnectDevice();

In the DeviceProvider class, you can add a new method:

Future<void> disconnectDevice() async {
  await SharedPreferencesUtil().btDeviceStructSet(BTDeviceStruct(id: '', name: ''));
  SharedPreferencesUtil().deviceName = '';
  if (connectedDevice != null) {
    await _bleDisconnectDevice(connectedDevice!); // Assuming _bleDisconnectDevice is moved to DeviceProvider
  }
  setIsConnected(false);
  setConnectedDevice(null);
  updateConnectingStatus(false);
  context.read<OnboardingProvider>().stopFindDeviceTimer(); // This might need to be handled differently depending on your app's architecture
}

@mdmohsin7 mdmohsin7 marked this pull request as ready for review September 23, 2024 06:58
@mdmohsin7 mdmohsin7 changed the title Device Settings and Improvements Device Settings, Intercom Integration and Improvements Sep 23, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between 4331261 and 70e9996 commits.
Files selected (16)
  • app/android/app/build.gradle (3)
  • app/android/app/proguard-rules.pro (1)
  • app/android/build.gradle (2)
  • app/android/gradle/wrapper/gradle-wrapper.properties (1)
  • app/android/settings.gradle (1)
  • app/ios/Podfile (1)
  • app/ios/Runner.xcodeproj/project.pbxproj (9)
  • app/ios/Runner/Info.plist (1)
  • app/lib/env/dev_env.dart (1)
  • app/lib/env/env.dart (2)
  • app/lib/env/prod_env.dart (1)
  • app/lib/main.dart (4)
  • app/lib/pages/home/device.dart (3)
  • app/lib/pages/settings/device_settings.dart (1)
  • app/lib/pages/settings/page.dart (3)
  • app/lib/providers/device_provider.dart (2)
Files ignored due to filter (1)
  • app/pubspec.yaml
Files skipped from review due to trivial changes (5)
  • app/android/app/proguard-rules.pro
  • app/android/build.gradle
  • app/android/gradle/wrapper/gradle-wrapper.properties
  • app/android/settings.gradle
  • app/ios/Podfile
Review comments generated (33)
  • Review: 18
  • LGTM: 15

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

@@ -445,7 +445,7 @@
CODE_SIGN_ENTITLEMENTS = "Runner/RunnerDebug-prod.entitlements";
DEVELOPMENT_TEAM = 9536L8KLMP;
INFOPLIST_KEY_CFBundleDisplayName = Omi;
IPHONEOS_DEPLOYMENT_TARGET = 14.0;
IPHONEOS_DEPLOYMENT_TARGET = 15.0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The iOS deployment target has been updated from 14.0 to 15.0. This change will require all users to have at least iOS 15.0 installed on their devices to use the app. If there are still a significant number of users on iOS 14, this could potentially exclude them from being able to update or install the app. Please ensure that this change aligns with your user base and support strategy.

- 				IPHONEOS_DEPLOYMENT_TARGET = 14.0;
+ 				IPHONEOS_DEPLOYMENT_TARGET = 15.0;

Comment on lines +36 to +38
namespace "com.friend.ios"


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The namespace "com.friend.ios" seems to be added in the Android build.gradle file. This could potentially cause confusion as it's an iOS namespace being used in an Android context. If this is intentional and required for some reason, consider adding a comment explaining why this is necessary.

-    namespace "com.friend.ios"
+    // Using iOS namespace due to XYZ reason
+    namespace "com.friend.ios"

Comment on lines 123 to 126
dependencies {
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.8.10"
androidTestImplementation 'androidx.test:runner:1.2.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
implementation 'androidx.multidex:multidex:2.0.1'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The Kotlin dependency has been removed from the dependencies block. If Kotlin is no longer being used in the project, this is fine. However, if there are still parts of the project written in Kotlin, this will cause compilation errors. Please ensure that all Kotlin code has been properly migrated to Java or another language before removing this dependency.

@@ -2,6 +2,7 @@ import 'package:flutter/material.dart';
import 'package:friend_private/backend/auth.dart';
import 'package:friend_private/backend/preferences.dart';
import 'package:friend_private/main.dart';
import 'package:friend_private/pages/home/support.dart';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The import statement for SupportPage has been added. Ensure that the SupportPage is properly implemented and does not have any dependencies that could potentially break the application.

+ import 'package:friend_private/pages/home/support.dart';

@@ -13,6 +14,8 @@ import 'package:friend_private/utils/other/temp.dart';
import 'package:friend_private/widgets/dialog.dart';
import 'package:package_info_plus/package_info_plus.dart';

import 'device_settings.dart';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The import statement for DeviceSettings has been added. Make sure that DeviceSettings is correctly implemented and doesn't introduce any unwanted dependencies that could affect the stability of the application.

+ import 'device_settings.dart';

Comment on lines +162 to +174
const SizedBox(height: 8),
TextButton(
onPressed: () async {
await Intercom.instance.displayArticle('9907475-how-to-charge-the-device');
},
child: const Text(
'How to charge?',
style: TextStyle(
color: Colors.white,
decoration: TextDecoration.underline,
),
),
),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The addition of a TextButton that opens an Intercom article when pressed seems to be a good way to provide users with additional information. However, ensure that the hardcoded article ID '9907475-how-to-charge-the-device' is valid and the article content is up-to-date.

+               const SizedBox(height: 8),
+               TextButton(
+                 onPressed: () async {
+                   await Intercom.instance.displayArticle('9907475-how-to-charge-the-device');
+                 },
+                 child: const Text(
+                   'How to charge?',
+                   style: TextStyle(
+                     color: Colors.white,
+                     decoration: TextDecoration.underline,
+                   ),
+                 ),
+               ),
             ],
           ),
         );

Comment on lines +24 to +30
Future _bleDisconnectDevice(BTDeviceStruct btDevice) async {
var connection = await ServiceManager.instance().device.ensureConnection(btDevice.id);
if (connection == null) {
return Future.value(null);
}
return await connection.disconnect();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The _bleDisconnectDevice function does not handle the case when connection is null. It should throw an error or return a specific result indicating that the connection could not be established.

    if (connection == null) {
      return Future.value(null);
    }
+    else {
+      throw Exception('Unable to establish connection with device');
+    }
    return await connection.disconnect();

Comment on lines +34 to +36
WidgetsBinding.instance.addPostFrameCallback((_) {
context.read<DeviceProvider>().getDeviceInfo();
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

It's good practice to call super.initState() at the beginning of initState. This ensures that any code from the superclass is executed before your subclass code.

    WidgetsBinding.instance.addPostFrameCallback((_) {
      context.read<DeviceProvider>().getDeviceInfo();
    });
-    super.initState();
+    super.initState();

Comment on lines +125 to +141
await SharedPreferencesUtil().btDeviceStructSet(BTDeviceStruct(id: '', name: ''));
SharedPreferencesUtil().deviceName = '';
if (provider.connectedDevice != null) {
await _bleDisconnectDevice(provider.connectedDevice!);
}
provider.setIsConnected(false);
provider.setConnectedDevice(null);
provider.updateConnectingStatus(false);
context.read<OnboardingProvider>().stopFindDeviceTimer();
Navigator.of(context).pop();
Navigator.of(context).pop();
ScaffoldMessenger.of(context).showSnackBar(SnackBar(
content: Text(
'Your Friend is ${provider.connectedDevice == null ? "unpaired" : "disconnected"} 😔'),
));
MixpanelManager().disconnectFriendClicked();
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

This block of code seems to be doing a lot of operations related to disconnecting a device. Consider moving this logic into a separate method in the DeviceProvider class to improve readability and maintainability.

-                      await SharedPreferencesUtil().btDeviceStructSet(BTDeviceStruct(id: '', name: ''));
-                      SharedPreferencesUtil().deviceName = '';
-                      if (provider.connectedDevice != null) {
-                        await _bleDisconnectDevice(provider.connectedDevice!);
-                      }
-                      provider.setIsConnected(false);
-                      provider.setConnectedDevice(null);
-                      provider.updateConnectingStatus(false);
-                      context.read<OnboardingProvider>().stopFindDeviceTimer();
-                      Navigator.of(context).pop();
-                      Navigator.of(context).pop();
-                      ScaffoldMessenger.of(context).showSnackBar(SnackBar(
-                        content: Text(
-                            'Your Friend is ${provider.connectedDevice == null ? "unpaired" : "disconnected"}  😔'),
-                      ));
-                      MixpanelManager().disconnectFriendClicked();
+                      provider.disconnectDevice();

Comment on lines +155 to +196
List<Widget> deviceSettingsWidgets(DeviceInfo? deviceInfo, BTDeviceStruct? device, BuildContext context) {
return [
ListTile(
title: const Text('Device Name'),
subtitle: Text(device?.name ?? 'Friend'),
),
ListTile(
title: const Text('Device ID'),
subtitle: Text(device?.id ?? '12AB34CD:56EF78GH'),
),
GestureDetector(
onTap: () {
Navigator.push(
context,
MaterialPageRoute(
builder: (context) => FirmwareUpdate(
deviceInfo: deviceInfo!,
device: device,
),
),
);
},
child: ListTile(
title: const Text('Firmware Update'),
subtitle: Text(deviceInfo?.firmwareRevision ?? '1.0.2'),
trailing: const Icon(Icons.arrow_forward_ios),
),
),
ListTile(
title: const Text('Hardware Revision'),
subtitle: Text(deviceInfo?.hardwareRevision ?? 'XIAO'),
),
ListTile(
title: const Text('Model Number'),
subtitle: Text(deviceInfo?.modelNumber ?? 'Friend'),
),
ListTile(
title: const Text('Manufacturer Name'),
subtitle: Text(deviceInfo?.manufacturerName ?? 'Based Hardware'),
),
];
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The deviceSettingsWidgets function is tightly coupled with the UI. Consider moving this logic into a separate widget to improve modularity and testability.

- List<Widget> deviceSettingsWidgets(DeviceInfo? deviceInfo, BTDeviceStruct? device, BuildContext context) {
-   return [
-     ListTile(
-       title: const Text('Device Name'),
-       subtitle: Text(device?.name ?? 'Friend'),
-     ),
-     ListTile(
-       title: const Text('Device ID'),
-       subtitle: Text(device?.id ?? '12AB34CD:56EF78GH'),
-     ),
-     GestureDetector(
-       onTap: () {
-         Navigator.push(
-           context,
-           MaterialPageRoute(
-             builder: (context) => FirmwareUpdate(
-               deviceInfo: deviceInfo!,
-               device: device,
-             ),
-           ),
-         );
-       },
-       child: ListTile(
-         title: const Text('Firmware Update'),
-         subtitle: Text(deviceInfo?.firmwareRevision ?? '1.0.2'),
-         trailing: const Icon(Icons.arrow_forward_ios),
-       ),
-     ),
-     ListTile(
-       title: const Text('Hardware Revision'),
-       subtitle: Text(deviceInfo?.hardwareRevision ?? 'XIAO'),
-     ),
-     ListTile(
-       title: const Text('Model Number'),
-       subtitle: Text(deviceInfo?.modelNumber ?? 'Friend'),
-     ),
-     ListTile(
-       title: const Text('Manufacturer Name'),
-       subtitle: Text(deviceInfo?.manufacturerName ?? 'Based Hardware'),
-     ),
-   ];
- }
+ DeviceSettingsWidget(deviceInfo: deviceInfo, device: device);

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between 70e9996 and def9628 commits.
Files selected (6)
  • app/android/app/build.gradle (4)
  • app/android/app/proguard-rules.pro (2)
  • app/android/app/src/main/AndroidManifest.xml (1)
  • app/android/app/src/main/java/com/friend/ios/MyApp.kt (1)
  • app/lib/pages/settings/page.dart (3)
  • app/lib/services/notification_service.dart (2)
Files skipped from review due to trivial changes (1)
  • app/lib/pages/settings/page.dart
Review comments generated (10)
  • Review: 7
  • LGTM: 3

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

Comment on lines +32 to +35
# For Awesome Notification Plugin
-keep class com.google.common.reflect.TypeToken
-keep class * extends com.google.common.reflect.TypeToken

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The Proguard rules for the Awesome Notification Plugin are added, but it's not clear if this plugin is used in the project. If it's not used, these lines should be removed to avoid unnecessary rules.

- # For Awesome Notification Plugin
- -keep class com.google.common.reflect.TypeToken
- -keep class * extends com.google.common.reflect.TypeToken

If the plugin is indeed used, then this change is fine.

@@ -67,6 +67,7 @@

<!-- Add android:stopWithTask option only when necessary. -->
<application
android:name=".MyApp"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The android:name attribute has been added to the <application> tag in the AndroidManifest.xml file. This is used to specify a fully qualified class name that extends Application, which will be instantiated when your process is created. Here, it's set to .MyApp.

Please ensure that the MyApp class exists and extends from the Application class. Also, if there are any global state or singleton instances being initialized in this class, make sure they are thread-safe and properly managed to avoid memory leaks.

-     <application
+     <application
+         android:name=".MyApp"
          android:icon="@mipmap/ic_launcher"
          android:label="@string/app_name"
          android:requestLegacyExternalStorage="true"

@@ -124,6 +125,7 @@ class NotificationService {
if (token == null) return;
String timeZone = await getTimeZone();
if (FirebaseAuth.instance.currentUser != null && token.isNotEmpty) {
await Intercom.instance.sendTokenToIntercom(token);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The sendTokenToIntercom method from the Intercom instance is being called with the FCM token as an argument. This is used to register the device's FCM token with Intercom, enabling push notifications.

+ await Intercom.instance.sendTokenToIntercom(token);

However, it would be a good idea to add error handling here. If the Intercom service fails or there's a network issue, this could throw an exception which isn't currently being caught.

try {
  await Intercom.instance.sendTokenToIntercom(token);
} catch (e) {
  print('Failed to send FCM token to Intercom: $e');
}

This will ensure that any issues with sending the token to Intercom don't cause the entire function to fail and provide some debugging information if something goes wrong.

Comment on lines +36 to +42
buildFeatures {
buildConfig = true
}

namespace "com.friend.ios"


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The buildFeatures block and the namespace field have been added. The buildFeatures block is used to enable or disable specific build features. Here, buildConfig is set to true which means a BuildConfig class will be generated in your APK file. This class contains constants that are useful at runtime for your app.

The namespace field is used to specify the package name for the generated R.java and Manifest.java files. It's good to see that you've specified it here, but make sure that this namespace matches with your application's package name.

+    buildFeatures {
+        buildConfig = true
+    }
+
+    namespace "com.friend.ios"

Comment on lines +86 to +87
buildConfigField("String", "INTERCOM_APP_ID", "\"${System.getenv("intercom.app.id")}\"")
buildConfigField("String", "INTERCOM_ANDROID_API_KEY", "\"${System.getenv("intercom.android.api.key")}\"")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

You're using environment variables to store sensitive data such as the Intercom App ID and API key. This is a good practice as it keeps these values out of the codebase, reducing the risk of accidental exposure. However, ensure that these environment variables are properly set in the build environment where this code will run.

+        buildConfigField("String", "INTERCOM_APP_ID", "\"${System.getenv("intercom.app.id")}\"")
+        buildConfigField("String", "INTERCOM_ANDROID_API_KEY", "\"${System.getenv("intercom.android.api.key")}\"")

@@ -105,6 +114,7 @@ android {
shrinkResources true
}
debug{
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

You've added ProGuard rules file to the debug build type. While this is not an issue per se, it's unusual to use ProGuard in debug builds as it obfuscates the code, making debugging more difficult. If you have a specific reason for doing this, it's fine. Otherwise, consider removing it from the debug build type.

+            proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'

Comment on lines 130 to +135
androidTestImplementation 'androidx.test:runner:1.2.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
implementation 'androidx.multidex:multidex:2.0.1'
implementation 'com.squareup.okhttp3:okhttp:4.12.0'
implementation 'androidx.constraintlayout:constraintlayout:2.0.0-alpha3'
implementation 'io.intercom.android:intercom-sdk:latest.release'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

You've added new dependencies and removed the Kotlin standard library dependency. The Intercom SDK has been added for chat support, which aligns with the PR description. However, the removal of the Kotlin standard library might cause issues if there are any Kotlin files in your project. If you're not using Kotlin at all, then it's fine to remove this dependency.

-    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.8.10"
+    implementation 'androidx.constraintlayout:constraintlayout:2.0.0-alpha3'
+    implementation 'io.intercom.android:intercom-sdk:latest.release'

@josancamon19 josancamon19 merged commit f01db3f into main Sep 24, 2024
1 check passed
@josancamon19 josancamon19 deleted the issue-887 branch September 24, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants