Intercom analytics #932
Intercom analytics #932
Conversation
|
WalkthroughThis update introduces the Changes
Uplevel your code reviews with Entelligence.AI ProEntelligence.AI ProIf 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (13)
- app/lib/main.dart (3)
- app/lib/pages/home/device.dart (2)
- app/lib/pages/home/page.dart (2)
- app/lib/pages/onboarding/name/name_widget.dart (3)
- app/lib/pages/onboarding/permissions/permissions_widget.dart (2)
- app/lib/pages/onboarding/welcome/page.dart (2)
- app/lib/pages/onboarding/wrapper.dart (5)
- app/lib/pages/settings/about.dart (2)
- app/lib/pages/settings/device_settings.dart (2)
- app/lib/providers/home_provider.dart (2)
- app/lib/providers/onboarding_provider.dart (2)
- app/lib/utils/analytics/analytics_manager.dart (1)
- app/lib/utils/analytics/intercom.dart (1)
Files skipped from review due to trivial changes (1)
- app/lib/pages/onboarding/welcome/page.dart
Review comments generated (29)
- Review: 22
- LGTM: 7
Tips
Chat with 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.AIin 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: ignoreanywhere in the PR description to pause further reviews from the bot.
| const SizedBox( | ||
| height: 12, | ||
| ), | ||
| InkWell( | ||
| child: Text( | ||
| 'Need Help?', | ||
| style: TextStyle( | ||
| color: Colors.grey.shade300, | ||
| decoration: TextDecoration.underline, | ||
| ), | ||
| ), | ||
| onTap: () { | ||
| Intercom.instance.displayMessenger(); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
The direct usage of Intercom.instance.displayMessenger(); is not consistent with the PR description which states that all Intercom related functionalities should be handled by the IntercomManager. Consider replacing this line with a method from IntercomManager to maintain consistency and encapsulation.
- Intercom.instance.displayMessenger();
+ IntercomManager.instance.displayMessenger();| androidApiKey: Env.intercomAndroidApiKey, | ||
| ); | ||
| } | ||
| await IntercomManager().initIntercom(); |
There was a problem hiding this comment.
The IntercomManager().initIntercom() is being called without checking if the Intercom App ID is null or not. In the old code, there was a null check for Env.intercomAppId. This check should be included in the new code to prevent potential issues when the app id is null.
- await IntercomManager().initIntercom();
+ if (Env.intercomAppId != null) {
+ await IntercomManager().initIntercom();
+ }| @@ -162,7 +162,7 @@ class _ConnectedDeviceState extends State<ConnectedDevice> { | |||
| const SizedBox(height: 8), | |||
| TextButton( | |||
| onPressed: () async { | |||
| await Intercom.instance.displayArticle('9907475-how-to-charge-the-device'); | |||
| await IntercomManager.instance.displayChargingArticle(); | |||
There was a problem hiding this comment.
The change from Intercom.instance.displayArticle to IntercomManager.instance.displayChargingArticle() is a good move towards encapsulation. However, it's not clear how the article ID is being managed in the new method. If the article ID is hard-coded within the displayChargingArticle method, this could lead to maintainability issues if the article ID changes in the future. Consider passing the article ID as a parameter or managing it through a configuration file.
- await IntercomManager.instance.displayChargingArticle();
+ await IntercomManager.instance.displayChargingArticle('9907475-how-to-charge-the-device');This suggestion assumes that you modify the displayChargingArticle method to accept an article ID as a parameter.
| AnalyticsManager().setUserAttribute('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled); | ||
| } | ||
| if (SharedPreferencesUtil().locationEnabled != await Permission.location.isGranted) { | ||
| SharedPreferencesUtil().locationEnabled = await Permission.location.isGranted; | ||
| MixpanelManager().setUserProperty('Location Enabled', SharedPreferencesUtil().locationEnabled); | ||
| AnalyticsManager().setUserAttribute('Location Enabled', SharedPreferencesUtil().locationEnabled); |
There was a problem hiding this comment.
While the change from MixpanelManager().setUserProperty to AnalyticsManager().setUserAttribute is correct, it's important to note that creating a new instance of AnalyticsManager every time you want to set an attribute can be inefficient. Consider making AnalyticsManager a singleton or providing it through dependency injection to avoid unnecessary object creation.
- AnalyticsManager().setUserAttribute('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled);
+ AnalyticsManager.instance.setUserAttribute('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled);
- AnalyticsManager().setUserAttribute('Location Enabled', SharedPreferencesUtil().locationEnabled);
+ AnalyticsManager.instance.setUserAttribute('Location Enabled', SharedPreferencesUtil().locationEnabled);| @@ -4,6 +4,7 @@ import 'package:flutter/material.dart'; | |||
| import 'package:friend_private/providers/onboarding_provider.dart'; | |||
| import 'package:friend_private/widgets/dialog.dart'; | |||
| import 'package:gradient_borders/box_borders/gradient_box_border.dart'; | |||
| import 'package:intercom_flutter/intercom_flutter.dart'; | |||
There was a problem hiding this comment.
The import statement for intercom_flutter is added, but it's not clear if the package has been added to the pubspec.yaml file. Please ensure that the intercom_flutter package is included in your project dependencies.
dependencies:
+ intercom_flutter: ^version| @@ -14,7 +14,7 @@ import 'package:friend_private/providers/device_provider.dart'; | |||
| import 'package:friend_private/services/devices.dart'; | |||
| import 'package:friend_private/services/notifications.dart'; | |||
| import 'package:friend_private/services/services.dart'; | |||
| import 'package:friend_private/utils/analytics/mixpanel.dart'; | |||
| import 'package:friend_private/utils/analytics/analytics_manager.dart'; | |||
There was a problem hiding this comment.
The import statement has been changed from MixpanelManager to AnalyticsManager. This is a good change as it abstracts the analytics service being used, allowing for easier changes in the future if needed. However, ensure that all instances of MixpanelManager have been replaced with AnalyticsManager throughout the codebase to maintain consistency.
- import 'package:friend_private/utils/analytics/mixpanel.dart';
+ import 'package:friend_private/utils/analytics/analytics_manager.dart';| @@ -62,20 +62,20 @@ class OnboardingProvider extends BaseProvider with MessageNotifierMixin implemen | |||
| void updateLocationPermission(bool value) { | |||
| hasLocationPermission = value; | |||
| SharedPreferencesUtil().locationEnabled = value; | |||
| MixpanelManager().setUserProperty('Location Enabled', SharedPreferencesUtil().locationEnabled); | |||
| AnalyticsManager().setUserAttribute('Location Enabled', SharedPreferencesUtil().locationEnabled); | |||
There was a problem hiding this comment.
The method call has been changed from setUserProperty to setUserAttribute. This seems like a reasonable change, but make sure that the setUserAttribute method in AnalyticsManager is implemented correctly and performs the same function as the previous setUserProperty method in MixpanelManager.
- MixpanelManager().setUserProperty('Location Enabled', SharedPreferencesUtil().locationEnabled);
+ AnalyticsManager().setUserAttribute('Location Enabled', SharedPreferencesUtil().locationEnabled);| notifyListeners(); | ||
| } | ||
|
|
||
| void updateNotificationPermission(bool value) { | ||
| hasNotificationPermission = value; | ||
| SharedPreferencesUtil().notificationsEnabled = value; | ||
| MixpanelManager().setUserProperty('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled); | ||
| AnalyticsManager().setUserAttribute('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled); |
There was a problem hiding this comment.
Similar to the above comment, ensure that the setUserAttribute method in AnalyticsManager is implemented correctly and performs the same function as the previous setUserProperty method in MixpanelManager.
- MixpanelManager().setUserProperty('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled);
+ AnalyticsManager().setUserAttribute('Notifications Enabled', SharedPreferencesUtil().notificationsEnabled);| notifyListeners(); | ||
| } | ||
|
|
||
| void updateBackgroundPermission(bool value) { | ||
| hasBackgroundPermission = value; | ||
| MixpanelManager().setUserProperty('Background Permission Enabled', hasBackgroundPermission); | ||
| AnalyticsManager().setUserAttribute('Background Permission Enabled', hasBackgroundPermission); |
There was a problem hiding this comment.
Again, ensure that the setUserAttribute method in AnalyticsManager is implemented correctly and performs the same function as the previous setUserProperty method in MixpanelManager.
- MixpanelManager().setUserProperty('Background Permission Enabled', hasBackgroundPermission);
+ AnalyticsManager().setUserAttribute('Background Permission Enabled', hasBackgroundPermission);| import 'package:friend_private/backend/preferences.dart'; | ||
| import 'package:friend_private/env/env.dart'; | ||
| import 'package:intercom_flutter/intercom_flutter.dart'; | ||
|
|
||
| class IntercomManager { | ||
| static final IntercomManager _instance = IntercomManager._internal(); | ||
| static IntercomManager get instance => _instance; | ||
| static final SharedPreferencesUtil _preferences = SharedPreferencesUtil(); | ||
|
|
||
| IntercomManager._internal(); | ||
|
|
||
| Intercom get intercom => Intercom.instance; | ||
|
|
||
| factory IntercomManager() { | ||
| return _instance; | ||
| } | ||
|
|
||
| Future<void> initIntercom() async { | ||
| if (Env.intercomAppId == null) return; | ||
| await intercom.initialize( | ||
| Env.intercomAppId!, | ||
| iosApiKey: Env.intercomIOSApiKey, | ||
| androidApiKey: Env.intercomAndroidApiKey, | ||
| ); | ||
| } | ||
|
|
||
| Future displayChargingArticle() async { | ||
| return await intercom.displayArticle('9907475-how-to-charge-the-device'); | ||
| } | ||
|
|
||
| Future logEvent(String eventName, {Map<String, dynamic>? metaData}) async { | ||
| return await intercom.logEvent(eventName, metaData); | ||
| } | ||
|
|
||
| Future updateCustomAttributes(Map<String, dynamic> attributes) async { | ||
| return await intercom.updateUser(customAttributes: attributes); | ||
| } | ||
|
|
||
| Future updateUser(String? email, String? name, String? uid) async { | ||
| return await intercom.updateUser( | ||
| email: email, | ||
| name: name, | ||
| userId: uid, | ||
| ); | ||
| } | ||
|
|
||
| Future<void> setUserAttributes() async { | ||
| await updateCustomAttributes({ | ||
| 'Notifications Enabled': _preferences.notificationsEnabled, | ||
| 'Location Enabled': _preferences.locationEnabled, | ||
| 'Plugins Enabled Count': _preferences.enabledPluginsCount, | ||
| 'Plugins Integrations Enabled Count': _preferences.enabledPluginsIntegrationsCount, | ||
| 'Speaker Profile': _preferences.hasSpeakerProfile, | ||
| 'Calendar Enabled': _preferences.calendarEnabled, | ||
| 'Recordings Language': _preferences.recordingsLanguage, | ||
| 'Authorized Storing Recordings': _preferences.permissionStoreRecordingsEnabled, | ||
| 'GCP Integration Set': _preferences.gcpCredentials.isNotEmpty && _preferences.gcpBucketName.isNotEmpty, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The IntercomManager class seems to be well implemented. It encapsulates the Intercom functionality and provides a single point of interaction with the Intercom SDK, which is a good practice for maintainability and modularity. However, there's a potential issue with the SharedPreferencesUtil instance being static. If the preferences change during the lifetime of the app, those changes won't be reflected in this instance. Consider making _preferences non-static and initialize it in the constructor or where it's needed.
- static final SharedPreferencesUtil _preferences = SharedPreferencesUtil();
+ final SharedPreferencesUtil _preferences;
- IntercomManager._internal();
+ IntercomManager._internal() : _preferences = SharedPreferencesUtil();Also, consider adding error handling to your async functions. Currently, if any of these operations fail (e.g., network issues when initializing Intercom), it could crash the application or lead to unexpected behavior. Wrapping these calls in try-catch blocks and handling errors appropriately would improve the robustness of your code.
For example:
Future<void> initIntercom() async {
try {
if (Env.intercomAppId == null) return;
await intercom.initialize(
Env.intercomAppId!,
iosApiKey: Env.intercomIOSApiKey,
androidApiKey: Env.intercomAndroidApiKey,
);
} catch (e) {
// Handle or log error
}
}Lastly, in the setUserAttributes method, you're directly accessing the preferences fields without checking if they have been initialized. This could potentially lead to null values being set as user attributes. Consider adding null checks before setting these attributes.
Summary by Entelligence.AI
IntercomManagerandAnalyticsManagerclasses to streamline analytics and user support functionalities, improving code maintainability.MixpanelManagerwithAnalyticsManagerfor setting user attributes related to notifications and location, ensuring consistency in analytics handling.