analytics events #80
analytics events #80
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on integrating analytics functionality across several components of the application. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RaulRinconX
left a comment
RaulRinconX
left a comment
There was a problem hiding this comment.
Albion online es un mmropg no lineal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
lib/core/utils/analytics_logger.dart (1)
27-34: Improve user ID retrieval efficiencyThe current implementation fetches user ID for every event. Consider caching the user ID or passing it during initialization.
+ static String? _cachedUserId; + static Future<String?> _getCurrentUserId() async { try { + if (_cachedUserId != null) return _cachedUserId; final String? userId = FirebaseAuth.instance.currentUser?.uid; + _cachedUserId = userId; return userId; } catch (e) { return null; } }lib/core/ui/layouts/main_layout.dart (1)
43-48: Move screen names to constantsThe screen names list should be defined as a constant to prevent recreation on each navigation and improve maintainability.
+ static const List<String> _screenNames = <String>[ + 'home', + 'cart', + 'orders', + 'profile', + ]; + void _onItemTapped(int index) { - final List<String> screenNames = <String>[ - 'home', - 'cart', - 'orders', - 'profile', - ];lib/features/splash/presentation/screens/splash_screen.dart (2)
Line range hint
35-44: Reduce code duplication in analytics loggingThe analytics logging code is duplicated between authenticated and unauthenticated paths. Consider extracting it to a helper method.
+ Future<void> _logSplashScreenEvent(bool authenticated, Duration loadTime) async { + try { + if (await networkInfo.isConnected) { + await AnalyticsLogger.logEvent( + eventName: 'splash_screen', + milliseconds: loadTime.inMilliseconds, + authenticated: authenticated, + ); + } + } catch (e) { + // Ignore logging errors when offline + } + } if (state is Authenticated) { - try { - if (await networkInfo.isConnected) { - await AnalyticsLogger.logEvent( - eventName: 'splash_screen', - milliseconds: loadTime.inMilliseconds, - authenticated: true, - ); - } - } catch (e) { - // Ignore logging errors when offline - } + await _logSplashScreenEvent(true, loadTime); Navigator.pushReplacementNamed(context, '/main'); } else if (state is Unauthenticated) { - try { - if (await networkInfo.isConnected) { - await AnalyticsLogger.logEvent( - eventName: 'splash_screen', - milliseconds: loadTime.inMilliseconds, - authenticated: false, - ); - } - } catch (e) { - // Ignore logging errors when offline - } + await _logSplashScreenEvent(false, loadTime); Navigator.pushReplacementNamed(context, '/login'); }Also applies to: 48-57
Line range hint
31-32: Consider caching network connectivity statusThe network check is performed for each analytics event. Consider caching the network status or using a stream-based approach for better efficiency.
lib/features/cart/presentation/bloc/cart_bloc.dart (1)
83-92: Optimize quantity update analytics.The
firstWhereoperation to get previous quantity is redundant since we already have the item in scope.- 'previous_quantity': state.items - .firstWhere((CartItemData item) => item.id == event.itemId) - .quantity, + 'previous_quantity': item.quantity,lib/features/home/presentation/screens/home_screen.dart (1)
119-125: LGTM! Consider adding error handlingThe analytics integration for tab changes is well implemented, capturing both current and previous tab states.
Consider wrapping the analytics call in a try-catch to ensure tab navigation remains functional even if analytics logging fails:
if (_tabController.indexIsChanging) { context.read<HomeBloc>().add(TabChanged(_tabController.index)); - AnalyticsLogger.logEvent( - eventName: 'tab_change', - additionalData: <String, dynamic>{ - 'tab': _getTabName(_tabController.index), - 'previous_tab': _getTabName(_tabController.previousIndex), - }, - ); + try { + AnalyticsLogger.logEvent( + eventName: 'tab_change', + additionalData: <String, dynamic>{ + 'tab': _getTabName(_tabController.index), + 'previous_tab': _getTabName(_tabController.previousIndex), + }, + ); + } catch (e) { + debugPrint('Failed to log tab change: $e'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
lib/core/ui/layouts/main_layout.dart(3 hunks)lib/core/utils/analytics_logger.dart(1 hunks)lib/features/cart/presentation/bloc/cart_bloc.dart(5 hunks)lib/features/home/presentation/screens/home_screen.dart(3 hunks)lib/features/orders/presentation/screens/order_details_screen.dart(3 hunks)lib/features/splash/presentation/screens/splash_screen.dart(3 hunks)lib/features/support/presentation/widgets/ticket_form.dart(2 hunks)
🔇 Additional comments (6)
lib/core/utils/analytics_logger.dart (1)
14-21: Consider batch processing for analytics events
Direct Firestore writes for each event could impact performance under high load. Consider implementing batch processing or queuing mechanism for analytics events.
Let's check for potential high-frequency event logging:
lib/features/support/presentation/widgets/ticket_form.dart (2)
19-27: LGTM! Analytics implementation follows best practices.
The analytics integration effectively captures the ticket submission workflow while respecting user privacy by logging metadata (field lengths) instead of actual content.
Also applies to: 33-40, 45-52
33-40: Consider sanitizing error messages in analytics logs.
While logging errors is valuable, ensure that error messages don't contain sensitive information before logging them.
lib/features/cart/presentation/bloc/cart_bloc.dart (1)
28-34: LGTM! Comprehensive cart analytics implementation.
The analytics implementation effectively captures all cart operations with relevant business metrics and context.
Also applies to: 57-66, 153-160
lib/features/orders/presentation/screens/order_details_screen.dart (1)
32-41: LGTM! Analytics implementation for Order Again feature.
The analytics event captures all relevant business metrics and follows the established pattern.
lib/features/home/presentation/screens/home_screen.dart (1)
2-3: Verify the need for both analytics services
The file imports both analytics_logger.dart and analytics_service.dart. This could lead to confusion or duplication of analytics functionality.
| } catch (e) { | ||
| // Silently fail if logging fails | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for analytics failures
The current implementation silently fails, which could mask critical issues. Consider logging errors to a monitoring service or debug console to help identify and resolve integration problems.
} catch (e) {
- // Silently fail if logging fails
+ debugPrint('Analytics logging failed: $e');
+ // Consider reporting to error monitoring service
}Committable suggestion skipped: line range outside the PR's diff.
| AnalyticsLogger.logEvent( | ||
| eventName: 'navigation', | ||
| additionalData: <String, dynamic>{ | ||
| 'screen': screenNames[index], | ||
| 'previous_screen': screenNames[_currentIndex], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for analytics logging
Navigation should continue even if analytics logging fails. Consider wrapping the logging call in a try-catch block.
+ try {
AnalyticsLogger.logEvent(
eventName: 'navigation',
additionalData: <String, dynamic>{
'screen': screenNames[index],
'previous_screen': screenNames[_currentIndex],
},
);
+ } catch (e) {
+ debugPrint('Failed to log navigation event: $e');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AnalyticsLogger.logEvent( | |
| eventName: 'navigation', | |
| additionalData: <String, dynamic>{ | |
| 'screen': screenNames[index], | |
| 'previous_screen': screenNames[_currentIndex], | |
| }, | |
| ); | |
| try { | |
| AnalyticsLogger.logEvent( | |
| eventName: 'navigation', | |
| additionalData: <String, dynamic>{ | |
| 'screen': screenNames[index], | |
| 'previous_screen': screenNames[_currentIndex], | |
| }, | |
| ); | |
| } catch (e) { | |
| debugPrint('Failed to log navigation event: $e'); | |
| } |
| final CartItemData removedItem = | ||
| state.items.firstWhere((CartItemData item) => item.id == event.itemId); | ||
|
|
||
| AnalyticsLogger.logEvent( | ||
| eventName: 'remove_from_cart', | ||
| additionalData: <String, dynamic>{ | ||
| 'item_id': event.itemId, | ||
| 'item_name': removedItem.title, | ||
| 'business_id': removedItem.businessId, | ||
| 'quantity': removedItem.quantity, | ||
| 'price': removedItem.offerPrice, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Add null safety for removed item analytics.
The firstWhere operation could throw if the item isn't found. Consider using firstWhereOrNull for safer analytics logging.
- final CartItemData removedItem =
- state.items.firstWhere((CartItemData item) => item.id == event.itemId);
+ final CartItemData? removedItem =
+ state.items.firstWhereOrNull((CartItemData item) => item.id == event.itemId);
+
+ if (removedItem != null) {
+ AnalyticsLogger.logEvent(
+ eventName: 'remove_from_cart',
+ additionalData: <String, dynamic>{
+ 'item_id': event.itemId,
+ 'item_name': removedItem.title,
+ 'business_id': removedItem.businessId,
+ 'quantity': removedItem.quantity,
+ 'price': removedItem.offerPrice,
+ },
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final CartItemData removedItem = | |
| state.items.firstWhere((CartItemData item) => item.id == event.itemId); | |
| AnalyticsLogger.logEvent( | |
| eventName: 'remove_from_cart', | |
| additionalData: <String, dynamic>{ | |
| 'item_id': event.itemId, | |
| 'item_name': removedItem.title, | |
| 'business_id': removedItem.businessId, | |
| 'quantity': removedItem.quantity, | |
| 'price': removedItem.offerPrice, | |
| }, | |
| ); | |
| final CartItemData? removedItem = | |
| state.items.firstWhereOrNull((CartItemData item) => item.id == event.itemId); | |
| if (removedItem != null) { | |
| AnalyticsLogger.logEvent( | |
| eventName: 'remove_from_cart', | |
| additionalData: <String, dynamic>{ | |
| 'item_id': event.itemId, | |
| 'item_name': removedItem.title, | |
| 'business_id': removedItem.businessId, | |
| 'quantity': removedItem.quantity, | |
| 'price': removedItem.offerPrice, | |
| }, | |
| ); | |
| } |
| void _handleOrderAgain(BuildContext context) { | ||
| // Create a new order with the same items | ||
| final OrderModel newOrder = OrderModel( | ||
| id: '', // ID will be generated by Firestore | ||
| businessId: order.businessId, | ||
| businessName: order.businessName, | ||
| items: order.items, | ||
| totalAmount: order.totalAmount, | ||
| status: order_entity.OrderStatus.pending, | ||
| createdAt: DateTime.now(), | ||
| ); | ||
|
|
||
| // Log the Order Again feature usage | ||
| AnalyticsLogger.logEvent( | ||
| eventName: 'order_again', | ||
| additionalData: <String, dynamic>{ | ||
| 'businessId': order.businessId, | ||
| 'originalOrderId': order.id, | ||
| 'itemCount': order.items.length, | ||
| 'totalAmount': order.totalAmount, | ||
| }, | ||
| ); | ||
|
|
||
| // Dispatch create order event | ||
| context.read<OrderBloc>().add(CreateOrderEvent(order: newOrder)); | ||
|
|
||
| // Show a snackbar to indicate the order is being processed | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Creating new order...'), | ||
| duration: Duration(seconds: 2), | ||
| ), | ||
| ); | ||
|
|
||
| // Navigate back to the orders list | ||
| Navigator.of(context).pop(); | ||
| } |
There was a problem hiding this comment.
Add error handling for Order Again feature.
The _handleOrderAgain method should handle potential errors during order creation and provide appropriate user feedback.
void _handleOrderAgain(BuildContext context) {
+ try {
final OrderModel newOrder = OrderModel(
id: '', // ID will be generated by Firestore
businessId: order.businessId,
businessName: order.businessName,
items: order.items,
totalAmount: order.totalAmount,
status: order_entity.OrderStatus.pending,
createdAt: DateTime.now(),
);
AnalyticsLogger.logEvent(
eventName: 'order_again',
additionalData: <String, dynamic>{
'businessId': order.businessId,
'originalOrderId': order.id,
'itemCount': order.items.length,
'totalAmount': order.totalAmount,
},
);
context.read<OrderBloc>().add(CreateOrderEvent(order: newOrder));
ScaffoldMessenger.of(context).showSnackBar(
const SnackBar(
content: Text('Creating new order...'),
duration: Duration(seconds: 2),
),
);
Navigator.of(context).pop();
+ } catch (e) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(
+ content: Text('Failed to create order: $e'),
+ backgroundColor: Colors.red,
+ ),
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void _handleOrderAgain(BuildContext context) { | |
| // Create a new order with the same items | |
| final OrderModel newOrder = OrderModel( | |
| id: '', // ID will be generated by Firestore | |
| businessId: order.businessId, | |
| businessName: order.businessName, | |
| items: order.items, | |
| totalAmount: order.totalAmount, | |
| status: order_entity.OrderStatus.pending, | |
| createdAt: DateTime.now(), | |
| ); | |
| // Log the Order Again feature usage | |
| AnalyticsLogger.logEvent( | |
| eventName: 'order_again', | |
| additionalData: <String, dynamic>{ | |
| 'businessId': order.businessId, | |
| 'originalOrderId': order.id, | |
| 'itemCount': order.items.length, | |
| 'totalAmount': order.totalAmount, | |
| }, | |
| ); | |
| // Dispatch create order event | |
| context.read<OrderBloc>().add(CreateOrderEvent(order: newOrder)); | |
| // Show a snackbar to indicate the order is being processed | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Creating new order...'), | |
| duration: Duration(seconds: 2), | |
| ), | |
| ); | |
| // Navigate back to the orders list | |
| Navigator.of(context).pop(); | |
| } | |
| void _handleOrderAgain(BuildContext context) { | |
| try { | |
| // Create a new order with the same items | |
| final OrderModel newOrder = OrderModel( | |
| id: '', // ID will be generated by Firestore | |
| businessId: order.businessId, | |
| businessName: order.businessName, | |
| items: order.items, | |
| totalAmount: order.totalAmount, | |
| status: order_entity.OrderStatus.pending, | |
| createdAt: DateTime.now(), | |
| ); | |
| // Log the Order Again feature usage | |
| AnalyticsLogger.logEvent( | |
| eventName: 'order_again', | |
| additionalData: <String, dynamic>{ | |
| 'businessId': order.businessId, | |
| 'originalOrderId': order.id, | |
| 'itemCount': order.items.length, | |
| 'totalAmount': order.totalAmount, | |
| }, | |
| ); | |
| // Dispatch create order event | |
| context.read<OrderBloc>().add(CreateOrderEvent(order: newOrder)); | |
| // Show a snackbar to indicate the order is being processed | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Creating new order...'), | |
| duration: Duration(seconds: 2), | |
| ), | |
| ); | |
| // Navigate back to the orders list | |
| Navigator.of(context).pop(); | |
| } catch (e) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text('Failed to create order: $e'), | |
| backgroundColor: Colors.red, | |
| ), | |
| ); | |
| } | |
| } |
| String _getTabName(int index) { | ||
| final List<String> tabNames = <String>[ | ||
| 'for_you', | ||
| 'dietary', | ||
| 'restaurant', | ||
| 'ingredients', | ||
| 'store', | ||
| 'diary', | ||
| 'drink', | ||
| ]; | ||
| return tabNames[index]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and maintainability of tab name mapping
The current implementation has several potential issues:
- No bounds checking for the index parameter
- Tab names are duplicated between UI labels and analytics
- Hardcoded list could get out of sync with actual tabs
Consider this more robust implementation:
- String _getTabName(int index) {
- final List<String> tabNames = <String>[
- 'for_you',
- 'dietary',
- 'restaurant',
- 'ingredients',
- 'store',
- 'diary',
- 'drink',
- ];
- return tabNames[index];
- }
+ String _getTabName(int index) {
+ // Define tab names as static const to ensure consistency
+ static const Map<int, String> tabNames = <int, String>{
+ 0: 'for_you',
+ 1: 'dietary',
+ 2: 'restaurant',
+ 3: 'ingredients',
+ 4: 'store',
+ 5: 'diary',
+ 6: 'drink',
+ };
+
+ // Validate index and return a default value if invalid
+ if (!tabNames.containsKey(index)) {
+ debugPrint('Invalid tab index: $index');
+ return 'unknown';
+ }
+
+ return tabNames[index]!;
+ }This implementation:
- Prevents runtime errors from invalid indices
- Makes the mapping explicit and easier to maintain
- Provides logging for debugging issues
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String _getTabName(int index) { | |
| final List<String> tabNames = <String>[ | |
| 'for_you', | |
| 'dietary', | |
| 'restaurant', | |
| 'ingredients', | |
| 'store', | |
| 'diary', | |
| 'drink', | |
| ]; | |
| return tabNames[index]; | |
| } | |
| String _getTabName(int index) { | |
| // Define tab names as static const to ensure consistency | |
| static const Map<int, String> tabNames = <int, String>{ | |
| 0: 'for_you', | |
| 1: 'dietary', | |
| 2: 'restaurant', | |
| 3: 'ingredients', | |
| 4: 'store', | |
| 5: 'diary', | |
| 6: 'drink', | |
| }; | |
| // Validate index and return a default value if invalid | |
| if (!tabNames.containsKey(index)) { | |
| debugPrint('Invalid tab index: $index'); | |
| return 'unknown'; | |
| } | |
| return tabNames[index]!; | |
| } |
closes #79
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These changes aim to enhance user experience through better tracking and easier navigation within the app.