RegistrationCubit (KIDS-1597) #1212
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (20)
lib/features/family/features/registration/cubit/family_registration_custom.dart (1)
1-10: Consider adding additional states for complete registration flowThe current implementation only includes a success state, which might be insufficient for handling all registration scenarios. A typical registration flow needs to handle multiple states.
Consider adding these additional states:
sealed class FamilyRegistrationState { const FamilyRegistrationState(); const factory FamilyRegistrationState.initial() = FamilyRegistrationInitial; const factory FamilyRegistrationState.loading() = FamilyRegistrationLoading; const factory FamilyRegistrationState.success() = FamilyRegistrationSuccess; const factory FamilyRegistrationState.error(String message) = FamilyRegistrationError; } class FamilyRegistrationInitial extends FamilyRegistrationState { const FamilyRegistrationInitial(); } class FamilyRegistrationLoading extends FamilyRegistrationState { const FamilyRegistrationLoading(); } class FamilyRegistrationSuccess extends FamilyRegistrationState { const FamilyRegistrationSuccess(); } class FamilyRegistrationError extends FamilyRegistrationState { final String message; const FamilyRegistrationError(this.message); }lib/features/family/features/registration/widgets/registration_app_bar.dart (1)
5-16: Consider adding documentation for the isUS parameter.While the constructor and properties are well-structured, the
isUSparameter's purpose and impact on the AppBar's behavior could benefit from documentation comments.Add documentation above the constructor:
+/// A custom AppBar for registration screens. +/// +/// The [isUS] parameter controls the visibility of the leading widget, +/// hiding it for US-specific registration flows when true. const RegistrationAppBar({ super.key, this.title, this.actions = const [], this.leading, this.isUS = false, });lib/utils/stripe_helper.dart (1)
Line range hint
1-42: Consider documenting the architectural changes.Given the significant restructuring of Stripe-related functionality from registration to family features, it would be helpful to document this architectural decision.
Consider adding a comment block at the top of the file explaining:
- The relationship between StripeHelper and the family feature module
- The reasoning behind moving Stripe functionality from registration to family context
- Any implications for future development or maintenance
lib/features/family/features/registration/widgets/accept_policy_row_us.dart (4)
21-31: Optimize modal bottom sheet configuration.Consider using const constructor parameters where possible to improve performance.
showModalBottomSheet<void>( context: context, isScrollControlled: true, useSafeArea: true, - shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(20), - ), + shape: const RoundedRectangleBorder( + borderRadius: BorderRadius.all(Radius.circular(20)), + ), builder: (_) => const TermsAndConditionsDialog( typeOfTerms: TypeOfTerms.privacyPolicy, ), ),
34-41: Add semantic label to checkbox for accessibility.The checkbox should have a semantic label to improve accessibility for screen readers.
Checkbox( value: checkBoxValue, onChanged: onTap, + semanticsLabel: 'Accept data storage policy', side: const BorderSide( color: AppTheme.inputFieldBorderEnabled, width: 2, ), ),
8-64: Add documentation for the widget class.Missing documentation for the widget class and its parameters. This would help other developers understand the purpose and usage of this widget.
Add documentation above the class declaration:
/// A widget that displays a row with a checkbox and text for accepting /// the US privacy policy. /// /// The widget includes a checkbox that can be toggled, explanatory text, /// and an info icon that opens the privacy policy when tapped. /// /// Parameters: /// - [onTap]: Callback function triggered when the checkbox is toggled /// - [checkBoxValue]: Current state of the checkbox class AcceptPolicyRowUs extends StatelessWidget {
53-59: Add tooltip to info icon for better UX.The info icon should have a tooltip to indicate its purpose to users.
child: Icon( FontAwesomeIcons.circleInfo, size: 20, color: AppTheme.primary20, + semanticsLabel: 'View privacy policy', ),lib/features/family/features/registration/widgets/mobile_number_form_field.dart (2)
8-27: Add documentation for the widget class and its parameters.While the parameters are well-structured, adding documentation would improve code maintainability and IDE support.
Consider adding documentation like this:
/// A form field widget for inputting mobile numbers with country prefix selection. /// /// This widget combines a country prefix dropdown with a phone number input field. /// It handles both the prefix and number input separately through callbacks. /// /// Parameters: /// - [phone] - Controller for the phone number input field /// - [onPrefixChanged] - Callback when country prefix is changed /// - [onPhoneChanged] - Callback when phone number is changed /// - [selectedCountryPrefix] - Currently selected country prefix /// - [hintText] - Placeholder text for the phone input field /// - [validator] - Validation function for the phone number /// - [formatter] - Optional input formatters for the phone number class MobileNumberFormField extends StatelessWidget {
32-97: Extract constant values and improve responsiveness.Consider extracting padding and sizing values into named constants and implementing more responsive sizing logic.
+ // Consider moving these to a constants file + static const double _topPadding = 10.0; + static const double _horizontalPadding = 10.0; + static const double _verticalPadding = 5.0; + static const double _spacerWidth = 5.0; + static const double _menuHeightFactor = 0.3; @override Widget build(BuildContext context) { final size = MediaQuery.of(context).size; final locals = context.l10n; return Padding( - padding: const EdgeInsets.only(top: 10), + padding: const EdgeInsets.only(top: _topPadding), child: Row( children: [lib/features/family/features/registration/widgets/avatar_selection_bottomsheet.dart (1)
79-89: Consider making the show method more configurable.The show method implementation is solid, but consider making it more flexible by allowing customization of:
- Background color
- Border radius
- Modal behavior (isDismissible, enableDrag)
Example enhancement:
static void show( BuildContext context, String id, + { + Color backgroundColor = Colors.white, + double borderRadius = 10.0, + bool isDismissible = true, + bool enableDrag = true, + } ) { showModalBottomSheet<void>( context: context, isScrollControlled: true, + isDismissible: isDismissible, + enableDrag: enableDrag, shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(10), + borderRadius: BorderRadius.circular(borderRadius), ), - backgroundColor: Colors.white, + backgroundColor: backgroundColor, builder: (context) => AvatarSelectionBottomsheet(id: id), ); }lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (1)
6-6: Good architectural improvement moving credit card setup into family feature.Moving the credit card setup from registration to the family feature module improves the codebase organization by:
- Better feature isolation and cohesion
- Clear separation of concerns between registration and family-specific functionality
- More maintainable and scalable feature-based architecture
lib/features/family/features/creditcard_setup/pages/credit_card_details.dart (1)
Line range hint
53-57: Consider decoupling from RegistrationBloc.The widget initializes and depends on both
StripeCubitandRegistrationBloc, but the TODO indicates it's no longer used for registration. This creates unnecessary coupling between features.Consider:
- Creating a dedicated bloc/cubit for credit card setup in the family feature
- Removing the dependency on
RegistrationBloc- Moving success handling to the new bloc/cubit
- @override - void initState() { - super.initState(); - getIt<StripeCubit>().fetchSetupIntent(); - context.read<RegistrationBloc>().add(const RegistrationInit()); - } + @override + void initState() { + super.initState(); + getIt<StripeCubit>().fetchSetupIntent(); + }lib/features/registration/bloc/registration_bloc.dart (2)
Line range hint
19-33: Good architectural improvement.The consolidation of registration operations into
authRepositoryimproves the architecture by:
- Reducing duplication of responsibilities
- Maintaining clear separation of concerns
- Simplifying the dependency graph
Line range hint
236-266: Consider improving the Stripe account verification polling.The current polling mechanism has some potential issues:
- It could run for up to 257 trials (potentially hours) before giving up
- The delay time abruptly jumps from 5s to 60s after 16 trials
- There's no way to cancel the polling if the user navigates away
Consider implementing a more robust polling strategy:
Future<void> _onStripeSuccess( RegistrationStripeSuccess event, Emitter<RegistrationState> emit, ) async { var user = authCubit.state.user; - var trials = 1; - var delayTime = 5; + const maxAttempts = 30; // ~15 minutes maximum + const initialDelay = 5; + const maxDelay = 60; + var attempts = 0; + var delay = initialDelay; - while (user.tempUser && trials < 257) { + while (user.tempUser && attempts < maxAttempts) { await authCubit.refreshUser(emitAuthentication: event.emitAuthenticated); user = authCubit.state.user; - log('trial number $trials, delay time is $delayTime,\n user is temporary: ${user.tempUser}'); + log('Attempt ${attempts + 1}/$maxAttempts, delay: ${delay}s, user is temporary: ${user.tempUser}'); - if (trials > 16) { - delayTime = 60; + if (!user.tempUser) break; + + attempts++; + // Exponential backoff with max delay + if (attempts < maxAttempts) { + delay = min(delay * 1.5, maxDelay).toInt(); + await Future<void>.delayed(Duration(seconds: delay)); } - trials++; - await Future<void>.delayed(Duration(seconds: delayTime)); } if (user.tempUser == false) {lib/features/family/features/auth/data/family_auth_repository.dart (2)
Line range hint
260-290: Add error handling and documentation for the registration flow.The implementation could benefit from:
- Error handling for API calls and intermediate steps
- Documentation explaining the registration flow
- Cleanup mechanism for partial failures
Consider applying these improvements:
+ /// Registers a new user in the system through a multi-step process: + /// 1. Registers the user with the API + /// 2. Creates a session by logging in + /// 3. Stores the user extension data locally + /// + /// Throws: + /// - [APIException] if registration or login fails + /// - [StorageException] if local storage operations fail Future<void> registerUser({ required TempUser tempUser, required bool isNewUser, }) async { + try { /// register user final userGUID = await _apiService.registerUser(tempUser.toJson()); /// create session await login( tempUser.email, tempUser.password, ); final userExt = UserExt( email: tempUser.email, guid: userGUID, // ... other fields ... ); await _storeUserExt(userExt); + } catch (e, stackTrace) { + LoggingInfo.instance.error( + 'Failed to register user: ${e.toString()}', + methodName: stackTrace.toString(), + ); + // Attempt cleanup of partial registration if needed + await _cleanupFailedRegistration(tempUser.email); + rethrow; + } } + /// Cleans up any partial registration data in case of failure + Future<void> _cleanupFailedRegistration(String email) async { + try { + // Add cleanup logic here (e.g., remove partial session data) + await _prefs.remove(Session.tag); + await _prefs.remove(UserExt.tag); + } catch (e) { + // Log but don't throw as this is already in an error handler + LoggingInfo.instance.error('Failed to cleanup registration: $e'); + } + }
The registration flow changes require attention - parallel registration systems detected
The verification reveals that while the new
FamilyAuthRepository.registerUser()is properly integrated withFamilyRegistrationCubit, the oldRegistrationBlocis still actively used in multiple places:
- The old registration flow is still present and actively used in
lib/features/registration/- Two files in the family feature still import and use the old registration bloc:
lib/features/family/helpers/logout_helper.dartlib/features/family/features/creditcard_setup/pages/credit_card_details.dart- The router shows both registration flows are configured:
- Old flow:
Pages.registration.path- New family flow:
FamilyPages.registrationUS.nameThis indicates incomplete migration that could lead to inconsistent registration flows in the application.
🔗 Analysis chain
Line range hint
260-290: Verify integration with FamilyRegistrationCubit.Let's verify that the new registration flow is properly integrated with the cubit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the integration between FamilyRegistrationCubit and FamilyAuthRepository # Test 1: Verify the cubit's usage of registerUser echo "Checking FamilyRegistrationCubit implementation..." rg -A 5 "registerUser" "lib/features/family/features/registration/cubit/" # Test 2: Verify there are no lingering uses of the old registration flow echo "Checking for old registration flow usage..." rg -l "RegistrationBloc|RegistrationRepository" "lib/"Length of output: 1603
Script:
#!/bin/bash # Let's check the actual usage context in files still referencing old registration echo "Checking usage context in files with old registration references..." rg -C 5 "RegistrationBloc|RegistrationRepository" "lib/features/registration/" # Also check if there are any direct imports of the old registration components in family feature echo "Checking imports in family feature..." rg "import.*registration/bloc" "lib/features/family/" # Check the router to see how registration flows are handled echo "Checking router implementation..." rg -C 5 "registration|signup" "lib/app/routes/app_router.dart"Length of output: 20332
lib/shared/widgets/custom_navigation_drawer.dart (1)
Line range hint
1-365: Consider implementing route constants and navigation guards.The drawer contains multiple navigation paths with authentication checks. Consider implementing:
- Centralized route constants to avoid string literals
- Navigation guards/middleware to handle auth checks consistently
- A navigation service to encapsulate common navigation patterns
This would improve maintainability and reduce duplication of auth checking logic across different navigation paths.
lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart (2)
Line range hint
183-203: Consider differentiating between user cancellations and actual Stripe errors.While the current implementation logs all Stripe exceptions as info level, it might be better to differentiate between user-initiated cancellations and actual payment processing errors.
Consider refactoring the error handling:
} on StripeException catch (e, stackTrace) { await AnalyticsHelper.logEvent( eventName: AmplitudeEvents.editPaymentDetailsCanceled, ); - /* Logged as info as stripe is giving exception - when for example people close the bottomsheet. - So it's not a real error :) - */ - LoggingInfo.instance.info( - e.toString(), - methodName: stackTrace.toString(), - ); + if (e.error.code == 'cancelled') { + LoggingInfo.instance.info( + 'User cancelled payment sheet', + methodName: stackTrace.toString(), + ); + } else { + LoggingInfo.instance.error( + 'Stripe payment processing error: ${e.error.message}', + methodName: stackTrace.toString(), + ); + }
Line range hint
82-92: Update error message for emailUsed state.The
emailUsedstate currently shows the same error message asinvalidEmail(errorTldCheck), which might confuse users. Consider using a more appropriate message that clearly indicates the email is already in use.Consider updating the error message:
if (state.status == PersonalInfoEditStatus.emailUsed) { showDialog<void>( context: context, builder: (_) => WarningDialog( title: locals.invalidEmail, - content: locals.errorTldCheck, + content: locals.emailAlreadyInUse, onConfirm: () => context.pop(), ), ); }lib/features/give/pages/home_page.dart (1)
Line range hint
441-467: Consider using mounted check for animation flag cleanup.The animation flag cleanup in the
.thencallback might be called after the widget is disposed, potentially causing a "setState() called after dispose()" error.Apply this diff to add a mounted check:
pageController .animateToPage( index, duration: const Duration(milliseconds: 300), curve: Curves.easeInOut, ) .then( (_) { + if (!mounted) return; setState(() { isPageAnimationActive = false; }); }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
.ruby-version(0 hunks)lib/app/app.dart(0 hunks)lib/app/injection/injection.dart(1 hunks)lib/app/routes/app_router.dart(0 hunks)lib/features/account_details/cubit/edit_stripe_cubit.dart(0 hunks)lib/features/account_details/cubit/edit_stripe_state.dart(0 hunks)lib/features/account_details/pages/change_phone_number_bottom_sheet.dart(1 hunks)lib/features/account_details/pages/personal_info_edit_page.dart(1 hunks)lib/features/children/add_member/pages/family_member_form_page.dart(1 hunks)lib/features/children/details/pages/child_details_page.dart(1 hunks)lib/features/family/app/family_routes.dart(1 hunks)lib/features/family/app/injection.dart(2 hunks)lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart(1 hunks)lib/features/family/features/auth/data/family_auth_repository.dart(2 hunks)lib/features/family/features/creditcard_setup/pages/credit_card_details.dart(1 hunks)lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart(0 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart(1 hunks)lib/features/family/features/profiles/repository/profiles_repository.dart(0 hunks)lib/features/family/features/registration/cubit/family_registration_cubit.dart(1 hunks)lib/features/family/features/registration/cubit/family_registration_custom.dart(1 hunks)lib/features/family/features/registration/pages/us_signup_page.dart(5 hunks)lib/features/family/features/registration/widgets/accept_policy_row_us.dart(1 hunks)lib/features/family/features/registration/widgets/avatar_selection_bottomsheet.dart(1 hunks)lib/features/family/features/registration/widgets/mobile_number_form_field.dart(1 hunks)lib/features/family/features/registration/widgets/registered_check_animation.dart(1 hunks)lib/features/family/features/registration/widgets/registration_app_bar.dart(1 hunks)lib/features/family/features/registration/widgets/widgets.dart(1 hunks)lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart(2 hunks)lib/features/give/pages/home_page.dart(3 hunks)lib/features/registration/bloc/registration_bloc.dart(1 hunks)lib/features/registration/domain/registration_repository.dart(0 hunks)lib/shared/widgets/custom_navigation_drawer.dart(1 hunks)lib/utils/stripe_helper.dart(1 hunks)
💤 Files with no reviewable changes (8)
- .ruby-version
- lib/app/app.dart
- lib/app/routes/app_router.dart
- lib/features/account_details/cubit/edit_stripe_cubit.dart
- lib/features/account_details/cubit/edit_stripe_state.dart
- lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart
- lib/features/family/features/profiles/repository/profiles_repository.dart
- lib/features/registration/domain/registration_repository.dart
✅ Files skipped from review due to trivial changes (5)
- lib/features/account_details/pages/change_phone_number_bottom_sheet.dart
- lib/features/children/add_member/pages/family_member_form_page.dart
- lib/features/children/details/pages/child_details_page.dart
- lib/features/family/app/family_routes.dart
- lib/features/family/features/registration/widgets/widgets.dart
🧰 Additional context used
📓 Learnings (3)
lib/features/family/features/creditcard_setup/pages/credit_card_details.dart (2)
Learnt from: TammiLion
PR: givtnl/givt-app#1130
File: lib/features/registration/pages/credit_card_details_page.dart:9-10
Timestamp: 2024-10-04T11:55:07.552Z
Learning: Ensure to verify the existence of methods like `refreshData` in the `NavigationBarHomeCubit` class (`lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart`) before flagging them as missing, especially when they might be newly added in the current PR changes.
Learnt from: TammiLion
PR: givtnl/givt-app#1130
File: lib/features/registration/pages/credit_card_details_page.dart:9-10
Timestamp: 2024-10-08T16:00:11.438Z
Learning: Ensure to verify the existence of methods like `refreshData` in the `NavigationBarHomeCubit` class (`lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart`) before flagging them as missing, especially when they might be newly added in the current PR changes.
lib/features/family/features/registration/widgets/avatar_selection_bottomsheet.dart (2)
Learnt from: TammiLion
PR: givtnl/givt-app#1175
File: lib/features/registration/widgets/avatar_selection_bottomsheet.dart:20-20
Timestamp: 2024-10-17T13:13:41.180Z
Learning: In the Flutter project, for the file `lib/features/registration/widgets/avatar_selection_bottomsheet.dart`, the `AvatarsCubit` is not managed using `BlocProvider`, but is obtained using `getIt<AvatarsCubit>()`.
Learnt from: Daniela510
PR: givtnl/givt-app#1175
File: lib/features/registration/widgets/avatar_selection_bottomsheet.dart:15-15
Timestamp: 2024-10-17T14:00:49.736Z
Learning: In the Flutter project, minor inconsistencies in naming conventions, such as constructor names not matching class names (e.g., in `lib/features/registration/widgets/avatar_selection_bottomsheet.dart`), are considered nitpicks by Daniela510 and should not be flagged in future reviews.
lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart (1)
Learnt from: Daniela510
PR: givtnl/givt-app#1189
File: lib/features/family/features/topup/screens/topup_bottom_sheet.dart:42-42
Timestamp: 2024-10-21T10:12:15.521Z
Learning: In the `TopupSuccessBottomSheet` class (`lib/features/family/features/topup/screens/topup_success_bottom_sheet.dart`), the `onSuccess` callback is properly invoked.
🔇 Additional comments (29)
lib/features/family/features/registration/widgets/registration_app_bar.dart (2)
1-4: LGTM! Appropriate class hierarchy for a custom AppBar.
The class correctly extends StatelessWidget and implements PreferredSizeWidget, which is required for custom AppBar implementations.
33-34: LGTM! Correct implementation of PreferredSizeWidget.
The preferredSize getter correctly returns a constant size matching the standard AppBar height.
lib/utils/stripe_helper.dart (1)
4-4: LGTM! Verify import path consistency across the codebase.
The import path change aligns with the architectural reorganization moving Stripe functionality to the family feature module.
Let's verify that all imports of StripeCubit have been updated consistently:
✅ Verification successful
Import path migration is complete and consistent
All references to StripeCubit are using the new import path under the family/creditcard_setup feature module. No instances of the old import path (features/registration/cubit/stripe_cubit.dart) were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old import paths
rg "import.*features/registration/cubit/stripe_cubit.dart"
# Search for new import paths to confirm migration
rg "import.*features/family/features/creditcard_setup/cubit/stripe_cubit.dart"
Length of output: 740
lib/features/family/features/registration/widgets/registered_check_animation.dart (2)
5-13: LGTM! Widget declaration follows Flutter best practices.
The class declaration is well-structured with proper constructor implementation and key handling.
1-3:
Remove redundant import and verify icon usage.
The widgets.dart import is redundant as it's already included in material.dart. Additionally, let's verify the usage of common_icons.dart.
Apply this diff to remove the redundant import:
import 'package:flutter/material.dart';
-import 'package:flutter/widgets.dart';
import 'package:givt_app/shared/widgets/common_icons.dart';✅ Verification successful
Let me verify if these icons are actually used in the animation widget.
Remove redundant widgets.dart import
The common_icons.dart import is correctly used as the widget uses both registeredCheck and registeredCheckBackground icons in its implementation. However, the widgets.dart import is redundant since it's already included in material.dart.
Apply this diff:
import 'package:flutter/material.dart';
-import 'package:flutter/widgets.dart';
import 'package:givt_app/shared/widgets/common_icons.dart';🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the usage of registeredCheck and registeredCheckBackground icons
# from common_icons.dart in this file
# Test: Search for icon usage in the file
rg -A 2 "registeredCheck" "lib/shared/widgets/common_icons.dart"
Length of output: 497
Script:
#!/bin/bash
# Check the content of the animation widget file to see if these icons are used
rg -l "registeredCheck" "lib/features/family/features/registration/widgets/registered_check_animation.dart"
# Get the full content to analyze the implementation
cat "lib/features/family/features/registration/widgets/registered_check_animation.dart"
Length of output: 1679
lib/features/family/features/registration/widgets/accept_policy_row_us.dart (1)
1-7: LGTM: Imports are well-organized and necessary.
All imports are relevant to the widget's functionality, covering UI components, theming, and required dialogs.
lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart (3)
4-4: LGTM: Import path update aligns with package restructuring.
The migration of CreditCardDetails from registration to creditcard_setup package reflects proper separation of concerns.
13-17: LGTM: Constructor formatting enhances readability.
The vertical alignment and proper use of required/optional parameters follow Dart best practices.
33-48: LGTM: Well-structured navigation flow with proper analytics tracking.
The implementation correctly handles the navigation chain from credit card details to wallet top-up, with appropriate analytics tracking.
lib/features/family/features/registration/widgets/mobile_number_form_field.dart (1)
1-6: LGTM! Clean imports and well-defined type alias.
The imports are appropriate for the widget's functionality, and the type alias improves code readability.
lib/features/family/features/registration/widgets/avatar_selection_bottomsheet.dart (1)
1-18: LGTM! Clean imports and proper class declaration.
The imports are well-organized, and the class is properly declared with the required parameter.
lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (1)
6-6: LGTM! Verify consistent usage of new import path.
The import path change from registration to creditcard_setup feature looks good.
Let's verify that this change is consistent across the codebase:
✅ Verification successful
Import path migration completed successfully
The verification shows that:
- No instances of the old import path (
features/registration/pages/credit_card_details) remain in the codebase - The new import path (
features/family/features/creditcard_setup/pages/credit_card_details) is consistently used across three files:lib/features/family/features/topup/screens/enter_details_bottom_sheet.dartlib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dartlib/features/children/details/pages/child_details_page.dart
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify new import usage
echo "Checking for old import paths..."
rg "features/registration/pages/credit_card_details"
echo "Verifying new import path usage..."
rg "features/family/features/creditcard_setup/pages/credit_card_details"
Length of output: 805
lib/features/family/features/creditcard_setup/pages/credit_card_details.dart (2)
10-10: Consider relocating this file as per TODO comments.
The TODO comments correctly identify that this file should be moved since it's no longer used during registration. Consider moving it to the family app's creditcard_setup feature directory to better reflect its current usage.
Let's verify the current usage of this file:
Also applies to: 18-20
Line range hint 95-108: Verify NavigationBarHomeCubit.refreshData() implementation.
The error handling looks good with appropriate logging levels. However, let's verify the existence of the refreshData method in NavigationBarHomeCubit as mentioned in the learnings.
Let's check the implementation:
✅ Verification successful
Based on the search results, I can now generate the final response:
The refreshData() method exists and is correctly implemented in NavigationBarHomeCubit
The method is implemented in lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart and handles:
- Fetching profiles from the profiles repository
- Checking for family invite groups
- Getting profile picture URLs
- Performing initial checks
The error handling in the credit card setup is correctly using this method to refresh the navigation bar data after error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for refreshData method in NavigationBarHomeCubit
ast-grep --pattern 'class NavigationBarHomeCubit {
$$$
refreshData() {
$$$
}
$$$
}'
Length of output: 97
Script:
#!/bin/bash
# Let's try a broader search for NavigationBarHomeCubit class and any refresh-related methods
rg "class NavigationBarHomeCubit" -A 10
# Also search for any file containing NavigationBarHomeCubit
fd NavigationBarHomeCubit
# Search for any method with refresh in NavigationBarHomeCubit
rg "refresh.*\(" --type dart -A 5
Length of output: 38974
lib/app/injection/injection.dart (2)
18-18: LGTM! Import follows project conventions.
The new import for stripe_cubit.dart is properly organized with other feature-related imports.
Line range hint 264-268: Consider enhancing dependency management robustness.
The StripeCubit registration looks good, but consider these improvements:
- Add error handling for missing dependencies
- Document the dependency chain
Consider this enhancement:
..registerLazySingleton<StripeCubit>(
() => StripeCubit(
+ // Throws if AuthRepository is not registered
+ // Dependencies: AuthRepository
authRepository: getIt<AuthRepository>(),
),
);Let's verify the dependency registration order:
✅ Verification successful
Let me gather more information to verify the registration order between AuthRepository and StripeCubit.
Registration order is correct, but documentation can be improved
The AuthRepository is registered before StripeCubit in the dependency chain, which is the correct order. The basic implementation is sound, and the suggestion to add documentation for clarity remains valid.
- The registration of AuthRepository appears earlier in the file
- StripeCubit is registered later and can safely access AuthRepository through GetIt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AuthRepository is registered before StripeCubit
rg -A 5 "registerLazySingleton<AuthRepository>" lib/app/injection/injection.dart
Length of output: 207
Script:
#!/bin/bash
# Search for StripeCubit registration to compare ordering
rg -B 10 -A 5 "registerLazySingleton<StripeCubit>" lib/app/injection/injection.dart
Length of output: 390
lib/features/family/app/injection.dart (1)
32-32: LGTM! Import statement follows project conventions.
The import statement is correctly formatted and properly organized within the imports section.
lib/features/registration/bloc/registration_bloc.dart (2)
Line range hint 144-185: Well-structured error handling pattern.
The error handling implementation is robust with:
- Specific handling of HTTP status codes (409, 400)
- Proper logging with stack traces
- Clear state transitions for different error scenarios
19-19: Verify migration of registration completion logic.
The removal of registrationRepository from the constructor suggests that registration completion logic has been moved elsewhere. This aligns with the introduction of FamilyRegistrationCubit mentioned in the PR summary.
Let's verify the migration of registration completion logic:
✅ Verification successful
Registration completion logic has been properly migrated
The verification shows that the registration completion logic has been correctly migrated:
FamilyRegistrationCubitnow handles the registration process through thesavePersonalInfomethod- Upon successful registration, it emits
FamilyRegistrationCustomSucces - The completion UI is handled by
RegistrationCompletedPagewhich shows success message and redirects to home - The removal of
registrationRepositoryfromRegistrationBlocis justified as the functionality has been consolidated in the family-specific implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for registration completion logic in FamilyRegistrationCubit
ast-grep --pattern 'class FamilyRegistrationCubit {
$$$
finishedRegistrationFlow($_) {
$$$
}
$$$
}'
# Search for any remaining references to registrationRepository
rg "registrationRepository" -A 5
Length of output: 146
Script:
#!/bin/bash
# Let's try a broader search for FamilyRegistrationCubit class
ast-grep --pattern 'class FamilyRegistrationCubit'
# Search for any file containing FamilyRegistrationCubit
rg "FamilyRegistrationCubit" -A 5
# Let's also check for any registration completion related methods
rg "finishedRegistration|completeRegistration|registrationComplete" -A 5
# Search for files with registration in their name
fd "registration" --type f
Length of output: 5485
Script:
#!/bin/bash
# Let's examine the FamilyRegistrationCubit implementation
cat lib/features/family/features/registration/cubit/family_registration_cubit.dart
# Check the registration completion page
cat lib/features/registration/pages/registration_completed_page.dart
# Look for any registration completion methods in the family registration cubit
rg -B 3 -A 10 "complete|finish|success" lib/features/family/features/registration/cubit/family_registration_cubit.dart
Length of output: 4073
lib/features/family/features/auth/data/family_auth_repository.dart (1)
46-50: LGTM! Clean interface addition.
The new registerUser method signature is well-designed with clear parameters and appropriate return type.
lib/features/account_details/pages/personal_info_edit_page.dart (2)
104-122: LGTM! Clean implementation of the header section.
The new header section improves user experience by providing clear context about the personal information page. The implementation follows Flutter best practices with proper spacing and text styling.
Line range hint 1-300: Verify complete removal of US-specific logic.
The code shows clean removal of US-specific conditionals, but let's verify there are no remaining references to ensure proper cleanup.
✅ Verification successful
The initial results show several US-specific references still present in the codebase. Let's verify if these are properly separated from the main personal info edit page and check for any remaining dependencies.
Let's verify how the navigation and routing is handled between the US and non-US personal info pages to ensure proper separation.
Based on the results, I can now generate the final response as we have sufficient evidence showing that US-specific code has been properly separated into its own component (USPersonalInfoEditPage) and the main PersonalInfoEditPage has been cleaned up.
US-specific code has been properly separated and cleaned up
The review comment's concern about US-specific code cleanup has been verified:
- The main
PersonalInfoEditPagehas been cleaned up and only contains UK-specific checks - US-specific functionality has been properly moved to a separate
USPersonalInfoEditPagecomponent under the family feature - The navigation between US and non-US versions is handled cleanly through the navigation bar home screen based on user country
The code changes are safe and maintain proper separation of concerns between US and non-US functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining US-specific conditionals or references
rg -i "isUSUser|US_COUNTRY|United\s*States" --type dart
# Search for any Stripe-related code that might need cleanup
rg "Stripe" --type dart
# Look for any TODO or FIXME comments related to US handling
rg -i "TODO.*\b(US|United States)\b" --type dart
Length of output: 9131
Script:
#!/bin/bash
# Check for US-specific code in the personal info edit page and related files
rg -B2 -A2 "isUsUser" lib/features/account_details/
# Check for any US-specific routing or navigation logic
ast-grep --pattern 'if ($user.isUsUser) { $$$ }'
# Check for any US-specific UI components or widgets
fd -e dart -x grep -l "us_personal_info" {}
# Look for any US-specific models or data structures
rg "United.*States|US_COUNTRY" lib/core/
Length of output: 285
Script:
#!/bin/bash
# Check the content of the US-specific navigation file
cat lib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dart
# Check for any references to personal info edit page in the navigation
rg -A3 "PersonalInfoEditPage" lib/features/family/
# Look for any conditional routing based on country
rg -A3 "navigatingFromFamily.*=.*true" lib/features/account_details/
Length of output: 11020
lib/shared/widgets/custom_navigation_drawer.dart (1)
53-53: LGTM! Registration navigation has been simplified.
The simplified navigation to the registration page aligns well with the broader refactoring of registration-related features.
lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart (2)
Line range hint 204-270: LGTM! Robust security implementation for sensitive operations.
The code properly implements biometric authentication for sensitive operations:
- Platform-specific handling of Face ID and fingerprint
- Authentication required for unregistration
- Proper visibility control based on available authentication methods
18-18: LGTM! Verify import path consistency.
The import change aligns with better feature organization by moving payment-related functionality to a dedicated creditcard_setup feature.
Let's verify the consistency of the new import path across the codebase:
✅ Verification successful
Import path migration is complete and consistent
The verification shows:
- No instances of old imports from the registration feature
- Consistent usage of the new import path
package:givt_app/features/family/features/creditcard_setup/cubit/stripe_cubit.dartacross all files:- lib/utils/stripe_helper.dart
- lib/app/injection/injection.dart
- lib/features/family/features/creditcard_setup/pages/credit_card_details.dart
- lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old imports of StripeCubit from registration feature
# and verify the new import path usage
# Test 1: Search for old imports (should return no results)
echo "Checking for old StripeCubit imports..."
rg "import.*registration.*stripe_cubit"
# Test 2: Verify consistent usage of new import path
echo "Checking usage of new StripeCubit import path..."
rg "import.*creditcard_setup.*stripe_cubit"
Length of output: 882
lib/features/give/pages/home_page.dart (2)
184-187: Verify the simplified registration dialog conditions.
The registration dialog conditions have been significantly simplified to only check !auth.user.needRegistration || auth.user.mandateSigned. Please verify that removing the previous US user and invite-specific checks aligns with the intended business logic.
Consider documenting the reasoning behind this simplification in a comment to help future maintainers understand the decision.
Line range hint 254-277: LGTM! Well-structured navigation implementation.
The changes improve the dialog's consistency and implement type-safe routing with proper parameter passing.
lib/features/family/features/registration/cubit/family_registration_cubit.dart (1)
59-62: Verify the 'isNewUser' parameter in 'registerUser'
In the registerUser call, isNewUser is set to false. If this method is used for new user registration, should isNewUser be true?
Run the following script to verify the usage of isNewUser:
This script will help determine if isNewUser should be true in this context.
✅ Verification successful
Based on the search results, I can now generate the final response as we have enough context about how isNewUser is used across the codebase.
The isNewUser: false parameter is correct in this context
Looking at the codebase usage patterns:
- In
auth_cubit.dart,isNewUser: trueis used for unregistered users - In both
registration_bloc.dartandfamily_registration_cubit.dart,isNewUser: falseis consistently used - The parameter is passed through to the API but doesn't affect the core registration logic, as seen in
api_service.registerUser()which only takes the user data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how 'isNewUser' is used in 'registerUser' calls.
# Search for the 'registerUser' method definition to understand the parameter's purpose.
rg 'Future<.*> registerUser\(' -A 5
# Search for all calls to 'registerUser' to see how 'isNewUser' is set elsewhere.
rg 'registerUser\(' -A 3
Length of output: 7483
lib/features/family/features/registration/pages/us_signup_page.dart (1)
59-59:
Use lowercase @override annotation.
In Dart, the @override annotation should be written in lowercase to adhere to the language conventions.
Apply this diff to correct the annotation:
-@Override
+@overrideLikely invalid or redundant comment.
lib/features/family/features/registration/cubit/family_registration_custom.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/cubit/family_registration_custom.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/widgets/registration_app_bar.dart
Show resolved
Hide resolved
lib/features/family/features/registration/widgets/registration_app_bar.dart
Show resolved
Hide resolved
lib/features/family/features/registration/widgets/registered_check_animation.dart
Show resolved
Hide resolved
lib/features/family/features/registration/cubit/family_registration_cubit.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/cubit/family_registration_cubit.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/cubit/family_registration_cubit.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/cubit/family_registration_cubit.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/registration/pages/us_signup_page.dart
Outdated
Show resolved
Hide resolved
…nt US are working
…com/givtnl/givt-app into feature/kids-1597-registrationcubit
…uthcubit from profilescubit.
Summary by CodeRabbit
New Features
FamilyRegistrationCubitfor managing family registration state.AcceptPolicyRowUswidget for privacy policy acceptance.AvatarSelectionBottomsheetfor avatar selection.MobileNumberFormFieldfor inputting mobile numbers with country prefix.RegisteredCheckAnimationfor animated checkmark display.RegistrationAppBarfor customizable app bar in registration screens.Bug Fixes
Refactor
RegistrationRepositoryand associated logic from multiple components.Chores