Auth Refactor (KIDS-1412) 🎉 #1243
Conversation
Allow easy access of user object in auth repo.
…interceptor-fixes Feature: change token retry policy to use correct repo (KIDS-1601)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Feature: Auth cubit part 2 of 3
…uthcubit from profilescubit.
RegistrationCubit (KIDS-1597)
WalkthroughThe pull request involves significant refactoring of the authentication system within the application. Key changes include the removal of the Changes
Assessment against linked issues
Suggested reviewers
Poem
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 65
🧹 Outside diff range and nitpick comments (108)
lib/features/family/features/registration/cubit/us_signup_custom.dart (1)
4-4: Fix typo in factory constructor nameThe factory constructor name has a typo:
successhould besuccess.- const factory UsSignupCustom.succes() = UsSignupCustomSuccess; + const factory UsSignupCustom.success() = UsSignupCustomSuccess;lib/features/email_signup/presentation/models/email_signup_uimodel.dart (1)
10-12: Add documentation and consider using a custom EmailAddress type.The fields would benefit from documentation and stronger type safety.
Consider:
- Adding documentation:
+ /// The email address entered by the user final String email; + /// The selected country for registration final Country country; + /// Whether the continue button should be enabled based on form validation final bool continueButtonEnabled;
- Creating a custom EmailAddress value object for better type safety:
// In a separate file: lib/core/value_objects/email_address.dart class EmailAddress { final String value; const EmailAddress._(this.value); static EmailAddress? create(String input) { if (!RegExp(r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$').hasMatch(input)) { return null; } return EmailAddress._(input); } }Then update the model:
- final String email; + final EmailAddress email;lib/shared/pages/redirect_to_browser_page.dart (1)
14-18: Consider adding loading state managementThe current implementation might lead to a race condition between URL launching and navigation. Consider managing the loading state to prevent premature navigation.
Consider using a state management solution or a simple boolean flag to track the URL launch status and ensure proper navigation timing. This would provide a more robust user experience.
lib/features/splash/cubit/splash_custom.dart (3)
1-10: Add documentation to explain the state hierarchy.The sealed class design effectively ensures type-safe state handling. Consider adding documentation to explain:
- The purpose of each redirection state
- When each state should be used
- The overall flow between states
Add this documentation above the class:
+/// Represents the different states of splash screen navigation. +/// +/// This sealed class hierarchy ensures type-safe handling of all possible +/// redirection states after the splash screen: +/// - [SplashRedirectToWelcome]: Initial welcome screen for new users +/// - [SplashRedirectToSignup]: Signup flow with pre-filled email +/// - [SplashRedirectToHome]: Main app home screen for authenticated users +/// - [SplashRedirectToAddMembers]: Add family members flow sealed class SplashCustom {
16-19: Consider adding email validation.While the implementation is clean, consider adding validation for the email parameter to ensure it's properly formatted before navigation.
class SplashRedirectToSignup extends SplashCustom { - const SplashRedirectToSignup(this.email); + const SplashRedirectToSignup(this.email) : assert(email.isNotEmpty); final String email; + + bool get isValidEmail => + RegExp(r'^[a-zA-Z0-9.]+@[a-zA-Z0-9]+\.[a-zA-Z]+').hasMatch(email); }
1-27: Well-structured implementation aligning with refactoring goals.The sealed class hierarchy effectively addresses the PR's objectives by:
- Providing a clear, type-safe representation of navigation states
- Eliminating complex conditional logic through pattern matching
- Separating navigation concerns from authentication logic
This creates a solid foundation for the auth refactoring.
Consider creating a companion
SplashStateclass in the future if you need to track additional metadata like loading status or error states.lib/features/family/features/registration/widgets/registration_app_bar.dart (2)
3-11: LGTM! Consider adding documentation.The class structure and constructor implementation are well-designed. However, adding documentation would help clarify the purpose of the
isUSflag and its impact on the app bar's behavior.Add documentation above the class:
+/// A custom app bar for the registration flow that handles different behaviors +/// for US and non-US contexts. +/// +/// The [isUS] flag determines whether to show the leading back button, which +/// is hidden for US users according to regional requirements. class RegistrationAppBar extends StatelessWidget
20-28: Consider more stable sizing approaches.Using
MediaQuery.size.heightfor image sizing can lead to layout inconsistencies across different screen sizes and orientations. Consider using fixed dimensions or more stable relative sizing approaches.- height: size.height * 0.03, + height: 32, // or another appropriate fixed sizeAdditionally, consider moving the asset path to a constants file for better maintainability.
// In a constants file: class AssetPaths { static const String registrationLogo = 'assets/images/logo.png'; }lib/features/email_signup/cubit/email_signup_custom.dart (2)
1-11: Add documentation to explain state transitions and usageThe sealed class provides a good foundation for type-safe state management, but documentation would help developers understand when each state should be used and the expected transitions between states.
Add documentation like this:
+/// Represents the various states of the email signup process. +/// +/// States: +/// - [EmailSignupCheckingEmail]: Initial state when validating email +/// - [EmailSignupShowFamilyRegistration]: Email is valid, show registration +/// - [EmailSignupShowFamilyLogin]: Email exists, show login +/// - [EmailSignupNoInternet]: Network connectivity issues +/// - [EmailSignupCertExpired]: Certificate validation failed sealed class EmailSignupCustom {
1-35: Consider regional authentication requirementsBased on the PR objectives mentioning different authentication requirements for EU and US regions, the current state hierarchy might need to be extended.
Consider these approaches:
- Add region-specific states if the flows differ significantly:
sealed class EmailSignupCustom { // ... existing states ... const factory EmailSignupCustom.euSpecificState(String email) = EmailSignupEUFlow; const factory EmailSignupCustom.usSpecificState(String email) = EmailSignupUSFlow; }
- Or, if the differences are minor, add a region parameter to existing states:
class EmailSignupShowFamilyRegistration extends EmailSignupCustom { const EmailSignupShowFamilyRegistration({ required this.email, required this.region, }); final String email; final AuthRegion region; // enum { eu, us } }lib/core/network/expired_token_retry_policy.dart (3)
1-10: Consider enhancing logging and documentationWhile the basic structure is good, consider these improvements for production readiness:
- Replace
dart:developerlogging with a more robust logging solution that supports different log levels and structured logging- Expand class documentation to specify:
- The exact conditions that trigger retries
- The behavior after max retries are exhausted
- Token refresh strategy
11-13: Consider making retry attempts configurableThe hard-coded value of 2 for
maxRetryAttemptsmight not be suitable for all environments. Consider:
- Making it configurable through dependency injection
- Adjusting based on environment (dev/staging/prod)
- int maxRetryAttempts = 2; + final int maxRetryAttempts; + + ExpiredTokenRetryPolicy({this.maxRetryAttempts = 2});
1-34: Consider regional authentication requirementsGiven the PR's objective to handle different EU/US authentication requirements, consider:
- Whether this retry policy needs region-specific behavior
- How this integrates with the mentioned
FamilyExpiredTokenRetryPolicy- Whether the
AuthRepositoryneeds to handle region-specific token refresh logicConsider implementing a factory pattern to create region-specific retry policies:
abstract class BaseExpiredTokenRetryPolicy extends RetryPolicy { factory BaseExpiredTokenRetryPolicy.create(Region region) { switch (region) { case Region.EU: return EUExpiredTokenRetryPolicy(); case Region.US: return USExpiredTokenRetryPolicy(); default: return ExpiredTokenRetryPolicy(); } } }lib/utils/stripe_helper.dart (2)
Line range hint
27-36: Consider EU market requirements for payment configurationGiven that the PR objectives mention addressing differences between EU and US requirements, the hardcoded US-specific values for Apple Pay and Google Pay configurations might need to be made configurable based on the market.
Consider:
- Making the country code and currency configurable based on the user's region
- Moving these configurations to a separate configuration file
- Adding EU-specific payment configurations if required
Line range hint
13-39: Enhance error handling for payment sheet operationsWhile the current implementation is functional, it could benefit from more robust error handling.
Consider applying this enhancement:
Future<PaymentSheetPaymentOption?> showPaymentSheet({ StripeResponse? stripe, }) async { stripe ??= getIt<StripeCubit>().state.stripeObject; + try { await Stripe.instance.initPaymentSheet( paymentSheetParameters: SetupPaymentSheetParameters( // ... existing parameters ... ), ); return Stripe.instance.presentPaymentSheet(); + } catch (e) { + debugPrint('Error setting up payment sheet: $e'); + rethrow; + } }lib/core/network/family_expired_token_retry_policy.dart (2)
1-11: Consider constructor-based dependency injection instead of service locatorWhile the implementation is clean, using
getItdirectly in the class creates a tight coupling with the service locator. Consider passing theFamilyAuthRepositorythrough the constructor for better testability and maintainability.class FamilyExpiredTokenRetryPolicy extends RetryPolicy { + final FamilyAuthRepository authRepository; + + FamilyExpiredTokenRetryPolicy(this.authRepository);
12-14: Consider making maxRetryAttempts configurableThe hard-coded value of 2 might not be suitable for all environments. Consider making this configurable through constructor injection or environment configuration.
class FamilyExpiredTokenRetryPolicy extends RetryPolicy { + final int maxAttempts; + + FamilyExpiredTokenRetryPolicy({ + this.maxAttempts = 2, + }); + @override - int maxRetryAttempts = 2; + int maxRetryAttempts = maxAttempts;lib/features/splash/cubit/splash_cubit.dart (1)
8-13: Add class-level documentation.Consider adding a class-level documentation comment explaining the purpose of this cubit and its role in the authentication flow.
+/// Manages the splash screen state and initialization flow. +/// Handles authentication checks and redirects users based on their auth state +/// and profile completion status. class SplashCubit extends CommonCubit<void, SplashCustom> {lib/features/family/features/login/presentation/models/family_login_sheet_custom.dart (2)
1-16: Documentation could be enhanced with more details.The sealed class design is excellent and aligns well with the authentication refactoring goals. Consider enhancing the documentation with:
- When each dialog state should be triggered
- The expected user journey through these states
- Integration examples with
FamilyLoginCubitExample enhancement:
/// Represents different dialog states during family login process. /// /// This sealed class hierarchy ensures type-safe handling of various /// login attempt scenarios: -/// - Two attempts remaining -/// - One attempt remaining -/// - General failure -/// - Account locked out -/// - Successful login +/// - Two attempts remaining: Shown after first failed attempt +/// - One attempt remaining: Shown after second failed attempt +/// - General failure: Shown for network/server errors +/// - Account locked out: Shown after third failed attempt +/// - Successful login: Triggers navigation to next screen /// /// Usage: /// ```dart -/// emitCustom(FamilyLoginSheetCustom.showTwoAttemptsLeftDialog()); +/// // In FamilyLoginCubit +/// if (loginAttempts == 1) { +/// emitCustom(FamilyLoginSheetCustom.showTwoAttemptsLeftDialog()); +/// } /// ```
18-29: Consider more concise naming for factory constructors.The factory constructors are well-structured and follow a logical flow. However, the naming could be more concise by removing redundant words.
- const factory FamilyLoginSheetCustom.showTwoAttemptsLeftDialog() = + const factory FamilyLoginSheetCustom.twoAttemptsLeft() = TwoAttemptsLeftDialog; - const factory FamilyLoginSheetCustom.showOneAttemptLeftDialog() = + const factory FamilyLoginSheetCustom.oneAttemptLeft() = OneAttemptLeftDialog; - const factory FamilyLoginSheetCustom.showFailureDialog() = FailureDialog; + const factory FamilyLoginSheetCustom.failure() = FailureDialog; - const factory FamilyLoginSheetCustom.showLockedOutDialog() = LockedOutDialog; + const factory FamilyLoginSheetCustom.lockedOut() = LockedOutDialog;lib/features/family/features/registration/widgets/registered_check_animation.dart (3)
1-3: Remove redundant importThe import of
widgets.dartis unnecessary as it's already included throughmaterial.dart.import 'package:flutter/material.dart'; -import 'package:flutter/widgets.dart'; import 'package:givt_app/shared/widgets/common_icons.dart';
28-30: Extract magic numbers into named constantsThe position and size calculations use magic numbers which make the code harder to maintain and understand. Consider extracting these values into named constants that explain their purpose.
+ // Animation multipliers for positioning and sizing + static const double _initialWidthFactor = 0.28; + static const double _widthAnimationOffset = 0.15; + static const double _initialHeightFactor = 0.4; + static const double _heightAnimationOffset = 0.25; + static const double _sizeAnimationFactor = 0.3; + @override Widget build(BuildContext context) { final size = MediaQuery.sizeOf(context); return TweenAnimationBuilder( duration: const Duration(seconds: 2), curve: Curves.elasticOut, tween: Tween<double>(begin: 0, end: 1), builder: (context, value, child) { - leftPosition = size.width * (0.28 - 0.15 * value); - topPosition = size.width * (0.4 - 0.25 * value); - imageSize = size.width * (0.3 * value); + leftPosition = size.width * (_initialWidthFactor - _widthAnimationOffset * value); + topPosition = size.width * (_initialHeightFactor - _heightAnimationOffset * value); + imageSize = size.width * (_sizeAnimationFactor * value);
15-49: Add documentation for animation behaviorConsider adding documentation to explain:
- The purpose and behavior of the animation
- The expected visual outcome
- The reasoning behind the positioning calculations
Add this documentation above the class:
+/// A widget that displays an animated checkmark for successful registration. +/// The animation starts with a background and animates a checkmark scaling +/// and positioning from the center outward using an elastic curve. +/// The final position and size are calculated relative to the screen dimensions +/// to ensure consistent appearance across different device sizes. class _RegisteredCheckAnimationState extends State<RegisteredCheckAnimation> {lib/features/family/helpers/logout_helper.dart (3)
Line range hint
39-41: Improve error handling and loggingThe current implementation silently catches all exceptions, which could mask critical issues. Consider adding error logging and potentially informing the user of logout failures.
} catch (e) { - // do nothing, even if logging out fails, from welcome page user can re-login + debugPrint('Logout failed: $e'); + // Consider showing a snackbar or dialog for critical failures + // while still proceeding to welcome page }
Line range hint
35-38: Consolidate state cleanup operationsMultiple independent state cleanup calls could be consolidated into a single coordinated operation to ensure consistent state reset.
Consider creating a dedicated service to handle all cleanup operations:
class AuthCleanupService { Future<void> cleanupAllState(BuildContext context) async { // Coordinate all cleanup operations await Future.wait([ context.read<ProfilesCubit>().logout(), context.read<FlowsCubit>().resetFlow(), context.read<RegistrationBloc>().add(const RegistrationReset()), ]); } }
Line range hint
15-19: Consider making logout function asyncThe function performs multiple operations that could be asynchronous. Making it async would allow proper handling of operation ordering and error cases.
- void logout( + Future<void> logout(lib/features/children/add_member/widgets/add_member_loading_page.dart (2)
Line range hint
26-36: Simplify redundant state handling logicThe current implementation has redundant conditional branches that all lead to the same navigation action. Only the error states show a message.
Consider simplifying the logic:
listener: (context, state) { - if (state.status == AddMemberStateStatus.success) { - _navigateToProfileSelection(context); - } else if (state.status == AddMemberStateStatus.error || - state.status == AddMemberStateStatus.topupFailed) { - _navigateToProfileSelection(context); - SnackBarHelper.showMessage(context, text: state.error); - } else if (state.status == AddMemberStateStatus.successCached) { - _navigateToProfileSelection(context); - } else { - _navigateToProfileSelection(context); - } + if (state.status == AddMemberStateStatus.error || + state.status == AddMemberStateStatus.topupFailed) { + SnackBarHelper.showMessage(context, text: state.error); + } + _navigateToProfileSelection(context); }
11-12: Consider additional improvements for robustness
- Add documentation to explain the widget's purpose and lifecycle.
- Consider implementing a loading timeout to handle cases where the operation takes too long.
Example documentation:
+/// A loading page displayed while adding a new family member. +/// Shows a loading indicator and handles navigation based on the operation's outcome. +/// Part of the family member addition flow. class AddMemberLoadingPage extends StatelessWidget {lib/features/family/features/auth/bloc/family_auth_cubit.dart (1)
Line range hint
1-58: Track technical debt from auth refactorWhile the transition approach using legacy support methods is reasonable, we should track this technical debt to ensure it's addressed in future iterations.
Would you like me to create a GitHub issue to track the following items?
- Migration plan for legacy getter and methods
- Comprehensive error handling implementation
- Async behavior verification
lib/features/auth/cubit/auth_state.dart (1)
38-38: Consider moving navigation logic out of AuthStateWhile the simplification is good, having navigation logic within the state class couples presentation concerns with state management. Consider:
- Moving navigation logic to a dedicated navigator service
- Using a route configuration pattern
- Implementing navigation through route events instead of direct function calls
This would:
- Improve testability by allowing navigation mocking
- Better separate concerns
- Make navigation flows more maintainable
Also applies to: 55-55
lib/features/family/features/registration/widgets/accept_policy_row_us.dart (2)
18-63: Enhance accessibility and user experience.Several improvements can be made to the widget:
- Missing accessibility labels for screen readers
- Hard-coded strings should be localized
- The entire row triggering the terms dialog might be confusing
- No visual feedback when tapping the info icon
Apply these improvements:
@override Widget build(BuildContext context) { - return GestureDetector( - onTap: () => showModalBottomSheet<void>( + return Row( + children: [ + Checkbox( + value: checkBoxValue, + onChanged: onTap, + side: const BorderSide( + color: AppTheme.inputFieldBorderEnabled, + width: 2, + ), + ), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Semantics( + label: 'Data storage permission checkbox', + child: BodySmallText.primary40( + // TODO: Move to localization + 'Givt is permitted to save my data', + ), + ), + ], + ), + ), + GestureDetector( + onTap: () => showModalBottomSheet<void>( context: context, isScrollControlled: true, useSafeArea: true, shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(20), ), builder: (_) => const TermsAndConditionsDialog( typeOfTerms: TypeOfTerms.privacyPolicy, ), ), - child: Row( - children: [ - Checkbox( - value: checkBoxValue, - onChanged: onTap, - side: const BorderSide( - color: AppTheme.inputFieldBorderEnabled, - width: 2, - ), - ), - Expanded( - child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - BodySmallText.primary40( - 'Givt is permitted to save my data', - ), - ], - ), - ), - const Padding( - padding: EdgeInsets.symmetric(horizontal: 8), - child: Icon( + child: Semantics( + button: true, + label: 'View privacy policy details', + child: Padding( + padding: EdgeInsets.symmetric(horizontal: 8), + child: Icon( FontAwesomeIcons.circleInfo, size: 20, color: AppTheme.primary20, + ), ), ), - ], - ), + ), + ], ); }
1-64: Good alignment with authentication refactoring objectives.This widget demonstrates good separation of concerns by:
- Isolating US-specific policy acceptance UI
- Following a clear presentation layer responsibility
- Contributing to the simplified authentication flow
Consider documenting the relationship between EU and US policy acceptance widgets in a central location to help future maintainers understand the differences.
lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart (1)
4-4: LGTM! Good architectural improvement.Moving credit card details to its own feature module improves separation of concerns and aligns with the PR's objective of better code organization.
lib/features/splash/pages/splash_page.dart (2)
21-36: Consider creating cubit in initState for better hot reload support.While the current implementation works, creating the cubit as a field might cause issues during hot reload. Consider moving the cubit initialization to initState:
- final _cubit = getIt<SplashCubit>(); + late final SplashCubit _cubit; @override void initState() { super.initState(); + _cubit = getIt<SplashCubit>(); WidgetsBinding.instance.addPostFrameCallback((_) { _cubit.init(); }); }
38-57: Enhance accessibility for loading state.While the loading UI is clean, consider adding semantic labels for better accessibility:
onLoading: (context) => Scaffold( body: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ - Image.asset( - 'assets/images/logo.png', - width: 100, - ), + Semantics( + label: 'Application Logo', + child: Image.asset( + 'assets/images/logo.png', + width: 100, + ), + ), const SizedBox(height: 20), - const CustomCircularProgressIndicator(), + const Semantics( + label: 'Loading application', + child: CustomCircularProgressIndicator(), + ), ], ), ),lib/shared/widgets/outlined_text_form_field.dart (3)
9-10: Consider improving parameter documentation and defaultsWhile making the controller optional improves flexibility, consider:
- Moving the comment about initialValue usage to proper documentation
- Evaluating if an empty string default is appropriate for all use cases
Consider this improvement:
- this.controller, - this.initialValue = '', // Only when controller is null + /// The controller for this text field. If null, [initialValue] will be used instead + this.controller, + /// The initial value to use when [controller] is null + this.initialValue = '',
26-27: Consider grouping related properties togetherThe controller and initialValue properties are closely related but separated from other text input properties. Consider grouping them with other text input related properties for better code organization.
46-46: Consider using a more concise null checkThe conditional initialValue assignment could be simplified using the null-aware operator.
- initialValue: controller == null ? initialValue : null, + initialValue: controller?.initialValue ?? initialValue,lib/features/family/features/registration/cubit/us_signup_cubit.dart (1)
50-51: Extract magic numbers into named constants.The amount limits (4999 and 499) should be extracted into named constants to improve maintainability and clarity.
+ private const US_AMOUNT_LIMIT = 4999; + private const DEFAULT_AMOUNT_LIMIT = 499; amountLimit: - country.toUpperCase() == Country.us.countryCode ? 4999 : 499, + country.toUpperCase() == Country.us.countryCode + ? US_AMOUNT_LIMIT + : DEFAULT_AMOUNT_LIMIT,lib/features/family/utils/family_auth_utils.dart (1)
15-15: Consider adding documentation for the authentication flow.While the method implementation is solid with proper error handling and context checks, adding documentation about the authentication flow (biometric vs. email/password) would improve maintainability.
Add this documentation above the method:
+ /// Attempts to authenticate the user using the following flow: + /// 1. If biometric authentication is available, attempt that first + /// 2. If biometric auth fails or is unavailable, fall back to email/password + /// 3. After successful authentication, execute the provided navigation callbacklib/features/family/app/family_pages.dart (1)
Line range hint
1-93: Consider standardizing path formats across all routes.The enum contains a mix of absolute paths (with leading slash) and relative paths. Consider standardizing all paths to either absolute or relative format for consistency and maintainability.
Examples of inconsistency:
- Absolute: '/registration-us', '/profile-selection'
- Relative: 'parent-give', 'wallet', 'history'
lib/utils/auth_utils.dart (1)
108-114: LGTM! Consider extracting email logic for better readabilityThe simplified LoginPage construction successfully removes US/EU conditional complexity while maintaining proper email handling functionality.
Consider extracting the email logic to improve readability:
- email: checkAuthRequest.email.isNotEmpty - ? checkAuthRequest.email - : context.read<AuthCubit>().state.user.email, + email: _resolveEmail(checkAuthRequest, context),Add this helper method:
String _resolveEmail(CheckAuthRequest request, BuildContext context) { return request.email.isNotEmpty ? request.email : context.read<AuthCubit>().state.user.email; }lib/features/family/features/topup/screens/empty_wallet_bottom_sheet.dart (1)
Line range hint
84-92: Update show method to include new parameterThe static
showmethod doesn't include theawaitActiveProfileBalanceparameter, making it inconsistent with the constructor changes.Update the method signature and usage:
static void show( BuildContext context, VoidCallback afterSuccessAction, + {bool awaitActiveProfileBalance = false}, ) { showModalBottomSheet<void>( context: context, isScrollControlled: true, shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(10), ), backgroundColor: Colors.white, builder: (context) => EmptyWalletBottomSheet( afterSuccessAction: afterSuccessAction, + awaitActiveProfileBalance: awaitActiveProfileBalance, ), ); }lib/features/family/features/registration/widgets/mobile_number_form_field.dart (3)
8-27: Add documentation for better maintainability.Consider adding documentation to improve code maintainability:
- Class-level documentation explaining the widget's purpose and usage
- Parameter documentation using /// comments
Example improvement:
+/// A form field widget for entering mobile phone numbers with country prefix selection. +/// +/// This widget combines a country prefix dropdown with a phone number input field, +/// providing a consistent way to collect phone numbers across the application. class MobileNumberFormField extends StatelessWidget { const MobileNumberFormField({ + /// Controller for the phone number input field required this.phone, + /// Callback when the country prefix is changed required this.onPrefixChanged, // ... add docs for other parameters
32-34: Consider extracting padding constants.The padding values are hardcoded in multiple places. Consider extracting them to named constants for better maintainability.
+const _kVerticalPadding = 5.0; +const _kHorizontalPadding = 10.0; +const _kTopPadding = 10.0; return Padding( - padding: const EdgeInsets.only(top: 10), + padding: const EdgeInsets.only(top: _kTopPadding), // ... contentPadding: const EdgeInsets.symmetric( - horizontal: 10, - vertical: 5, + horizontal: _kHorizontalPadding, + vertical: _kVerticalPadding, ),Also applies to: 85-88
8-99: Implementation aligns well with PR objectives.This widget demonstrates good separation of concerns and maintainable code structure:
- Clear responsibility for phone number input
- Flexible design supporting both EU and US requirements through formatters
- Reusable component that can be used across different registration flows
Consider documenting the expected formatter patterns for EU and US phone numbers to help other developers implement the correct validation rules.
lib/features/family/features/registration/widgets/avatar_selection_bottomsheet.dart (1)
79-89: Enhance bottom sheet presentation.Consider improving the bottom sheet's appearance and behavior:
showModalBottomSheet<void>( context: context, isScrollControlled: true, + useSafeArea: true, + isDismissible: true, + enableDrag: true, shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(10), ), backgroundColor: Colors.white, - builder: (context) => AvatarSelectionBottomsheet(id: id), + builder: (context) => FractionallySizedBox( + heightFactor: 0.9, + child: Padding( + padding: MediaQuery.of(context).viewInsets, + child: AvatarSelectionBottomsheet(id: id), + ), + ), );lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart (1)
112-117: Consider making the emission more conciseThe method could be simplified into a one-liner for better readability.
- void _emitData() { - emitData( - NavigationBarHomeScreenUIModel( - profilePictureUrl: profilePictureUrl, - familyInviteGroup: _familyInviteGroup, - ), - ); - } + void _emitData() => emitData(NavigationBarHomeScreenUIModel( + profilePictureUrl: profilePictureUrl, + familyInviteGroup: _familyInviteGroup, + ));lib/features/family/features/profiles/cubit/profiles_cubit.dart (3)
29-29: LGTM: Enhanced subscription type improves auth state handlingThe change from a boolean session check to a full
UserExtobject subscription provides more granular control and richer authentication state information, which aligns well with the refactoring goals.This change enables more sophisticated auth state handling and reduces the need for additional API calls to fetch user information separately.
Line range hint
31-46: Consider adding documentation for the auth state handlingThe authentication state handling logic would benefit from documentation explaining:
- The different states and their implications
- Why profile clearing behavior differs based on auth state
- The relationship between auth states and profile management
This would help maintain the codebase's clarity, especially given the complexity of EU/US authentication requirements mentioned in the PR objectives.
Add safeguards for handling rapid authentication state changes to prevent race conditions.
lib/features/family/features/profiles/cubit/profiles_cubit.dart:22-43lib/features/family/features/profiles/repository/profiles_repository.dart:22-43🔗 Analysis chain
Line range hint
37-46: Verify handling of rapid auth state changesWhile the authentication state handling logic is clear, consider potential race conditions:
- What happens if auth state changes while
fetchAllProfiles()is in progress?- Could rapid auth state changes lead to profile list inconsistencies?
Consider adding debouncing or ensuring operations are properly canceled when auth state changes.
Run this script to check for similar patterns in other cubits:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar auth state handling patterns that might need the same protection ast-grep --pattern 'authenticatedUserStream().listen( $$$ )'Length of output: 64
Script:
#!/bin/bash # Search for all implementations of authenticatedUserStream().listen to identify similar patterns rg 'authenticatedUserStream\(\)\.listen\s*\('Length of output: 297
Script:
#!/bin/bash # Search for all instances of authenticatedUserStream().listen with flexible whitespace rg 'authenticatedUserStream\(\)\.listen\s*\('Length of output: 297
Script:
#!/bin/bash # Extract 10 lines around each authenticatedUserStream().listen call in profiles_cubit.dart and profiles_repository.dart # Files to inspect FILES=( "lib/features/family/features/profiles/cubit/profiles_cubit.dart" "lib/features/family/features/profiles/repository/profiles_repository.dart" ) # Loop through each file and extract context for FILE in "${FILES[@]}"; do echo "### Context from $FILE ###" rg -C 5 'authenticatedUserStream\(\)\.listen\s*\(' "$FILE" echo "" doneLength of output: 1427
lib/features/family/features/creditcard_setup/pages/credit_card_details.dart (3)
10-10: Architectural improvement needed: Move file to appropriate feature moduleThe TODO comments correctly identify that this file needs to be relocated since it's now part of the family app flow rather than registration. This aligns with the PR's objective of improving separation of concerns.
Consider moving this file to:
lib/features/family/features/creditcard_setup/pages/if it's specific to family credit card setuplib/features/shared/pages/if it's shared between different featuresAlso applies to: 18-20
Line range hint
71-106: Refactor: Separate concerns in Stripe payment sheet handlingThe payment sheet handling combines multiple responsibilities: UI state management, analytics, navigation, and error handling. This makes the code harder to maintain and test.
Consider splitting this into smaller, focused methods:
- StripeHelper(context).showPaymentSheet().then((value) { - _handleStripeRegistrationSuccess(context); - final user = context.read<AuthCubit>().state.user; - AnalyticsHelper.setUserProperties( - userId: user.guid, - ); - unawaited( - AnalyticsHelper.logEvent( - eventName: AmplitudeEvents.registrationStripeSheetFilled, - eventProperties: AnalyticsHelper.getUserPropertiesFromExt( - user, - ), - ), - ); - }).onError((e, stackTrace) { - context.pop(); - final user = context.read<AuthCubit>().state.user; - unawaited( - AnalyticsHelper.logEvent(/*...*/) - ); - getIt<NavigationBarHomeCubit>().refreshData(); - LoggingInfo.instance.info(/*...*/); - }); + await _handlePaymentSheet(context);Create separate methods:
Future<void> _handlePaymentSheet(BuildContext context) async { try { await StripeHelper(context).showPaymentSheet(); await _onPaymentSuccess(context); } catch (e, stackTrace) { await _onPaymentError(context, e, stackTrace); } } Future<void> _onPaymentSuccess(BuildContext context) async { _handleStripeRegistrationSuccess(context); await _logPaymentSuccess(context); } Future<void> _onPaymentError(BuildContext context, dynamic error, StackTrace stackTrace) async { context.pop(); await _logPaymentError(context); getIt<NavigationBarHomeCubit>().refreshData(); LoggingInfo.instance.info( error.toString(), methodName: stackTrace.toString(), ); }
Line range hint
21-50: Consider implementing a dedicated payment serviceThe current implementation mixes UI, payment processing, analytics, and navigation logic in a single widget. To better align with the PR's objective of clear responsibility division, consider:
- Creating a dedicated payment service to handle Stripe interactions
- Moving analytics to a separate analytics service
- Using a coordinator pattern for navigation flow
This would make the code more maintainable and testable while reducing the widget's responsibilities.
Would you like me to provide a detailed example of this architecture?
lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (1)
Line range hint
31-41: Consider improving state management and browser handlingThe current implementation has several areas that could be improved:
- Browser state management relies on boolean flags which could lead to race conditions in edge cases
- Heavy reliance on global state access could make testing and maintenance more difficult
Consider:
- Moving browser state to a dedicated BLoC/Cubit
- Implementing a proper state machine for browser lifecycle
- Injecting dependencies through constructor instead of using global getIt
lib/shared/models/temp_user.dart (1)
52-55: Well-structured for EU/US requirementsThe class effectively handles both EU (IBAN) and US (AccountNumber + SortCode) banking details, which aligns well with the PR's objective of managing different regional requirements. The conditional serialization in
toJsonensures clean separation of these concerns.Consider documenting these regional differences in the class documentation to help future maintainers understand the design decisions.
lib/features/family/features/reset_password/presentation/pages/reset_password_sheet.dart (4)
Line range hint
27-37: Consider enhancing modal sheet presentation.While the implementation is functional, consider these improvements for better UX:
- Add
backgroundColor: Colors.transparentto remove the default background- Set
shapeparameter to add rounded corners- Consider adding
enableDrag: truefor intuitive dismissalreturn showModalBottomSheet<void>( context: context, isScrollControlled: true, useSafeArea: true, + backgroundColor: Colors.transparent, + enableDrag: true, + shape: const RoundedRectangleBorder( + borderRadius: BorderRadius.vertical(top: Radius.circular(20)), + ), builder: (_) => this, );
39-39: Consider extracting email validation logic.The email validation logic is currently embedded in the widget. Consider extracting it to a separate utility function for reusability and maintainability.
+ // In email_validator.dart + String? validateEmail(String? value, BuildContext context) { + if (value == null || value.isEmpty) { + return context.l10n.invalidEmail; + } + if (value.contains(Util.emailRegEx) == false) { + return context.l10n.invalidEmail; + } + return null; + } // In widget OutlinedTextFormField( controller: emailController, onChanged: (value) { setState(() { formKey.currentState!.validate(); }); }, keyboardType: TextInputType.emailAddress, - validator: (value) { - if (value == null || value.isEmpty) { - return context.l10n.invalidEmail; - } - if (value.contains(Util.emailRegEx) == false) { - return context.l10n.invalidEmail; - } - return null; - }, + validator: (value) => validateEmail(value, context), hintText: context.l10n.email, ),Also applies to: 82-94
Line range hint
39-57: Consider using AutoDisposeMixin for controller cleanup.The current manual disposal of controllers could be simplified using a mixin pattern.
+ mixin AutoDisposeMixin<T extends StatefulWidget> on State<T> { + final List<TextEditingController> _controllers = []; + + TextEditingController createController([String? initialText]) { + final controller = TextEditingController(text: initialText); + _controllers.add(controller); + return controller; + } + + @override + void dispose() { + for (final controller in _controllers) { + controller.dispose(); + } + super.dispose(); + } + } - class _ResetPasswordSheetState extends State<ResetPasswordSheet> { + class _ResetPasswordSheetState extends State<ResetPasswordSheet> with AutoDisposeMixin { final formKey = GlobalKey<FormState>(); - late TextEditingController emailController; + late final emailController = createController(widget.initialEmail); @override void initState() { super.initState(); - emailController = TextEditingController(text: widget.initialEmail); formKey.currentState?.validate(); } - @override - void dispose() { - emailController.dispose(); - super.dispose(); - }
Line range hint
143-167: Improve error handling specificity.The error state currently shows a generic message. Consider handling specific error cases differently.
onError: (context, message) { return FunBottomSheet( title: context.l10n.changePassword, icon: errorCircleWithIcon( circleSize: 140, iconData: FontAwesomeIcons.triangleExclamation, iconSize: 48, ), content: Column( children: [ BodyMediumText( - context.l10n.somethingWentWrong, + message ?? context.l10n.somethingWentWrong, textAlign: TextAlign.center, ), + if (message?.contains('network') ?? false) + BodyMediumText( + context.l10n.checkInternetConnection, + textAlign: TextAlign.center, + ), ], ),lib/features/family/features/profiles/repository/profiles_repository.dart (2)
Line range hint
90-98: Add error handling to auth stream listenerThe authentication stream listener should handle potential errors to prevent stream termination.
Consider updating the implementation:
_authRepository.authenticatedUserStream().listen( (user) { if (user != null) { _clearData(); refreshProfiles(); } else { _clearData(); } - }, + }, + onError: (error) { + LoggingInfo.instance.logExceptionForDebug(error); + _clearData(); + }, );
Line range hint
90-98: Consider optimizing state clearing logicThe
_clearData()method is called in both the authenticated and unauthenticated cases, which might be redundant.Consider consolidating the logic:
_authRepository.authenticatedUserStream().listen( (user) { + _clearData(); if (user != null) { - _clearData(); refreshProfiles(); - } else { - _clearData(); } }, );lib/core/network/request_helper.dart (3)
Line range hint
144-155: Add documentation for EU-specific behavior.While the implementation looks good, consider adding documentation to explain:
- EU-specific retry policy behavior
- When this client should be used vs US client
+ /// Creates an HTTP client configured for EU region with specific retry policies + /// and token handling for EU authentication flow. + /// @param client Optional pre-configured client to use instead of creating new one + /// @return Configured HTTP client for EU region Client createEUClient({Client? client}) {
156-166: Add documentation and consider reducing duplication.
- Add documentation for consistency with EU client
- Consider extracting common configuration to reduce duplication between EU/US clients
+ /// Creates an HTTP client configured for US region with specific retry policies + /// and token handling for US authentication flow. + /// @param client Optional pre-configured client to use instead of creating new one + /// @return Configured HTTP client for US region Client createUSClient({Client? client}) {Consider extracting common configuration:
Client _createRegionalClient({ required Client? client, required RetryPolicy retryPolicy, }) { return InterceptedClient.build( client: client, requestTimeout: const Duration(seconds: 30), interceptors: [ if (client == null) CertificateCheckInterceptor(), TokenInterceptor(), ], retryPolicy: retryPolicy, ); }Then simplify the regional methods:
Client createEUClient({Client? client}) { return _createRegionalClient( client: client, retryPolicy: ExpiredTokenRetryPolicy(), ); } Client createUSClient({Client? client}) { return _createRegionalClient( client: client, retryPolicy: FamilyExpiredTokenRetryPolicy(), ); }
Line range hint
128-143: Consider making default country selection more configurable.The
_checkCountrymethod currently defaults to 'NL' when no user extension is found. As the application grows internationally, consider:
- Making the default country configurable
- Potentially determining it based on device locale or user location
- Adding logging when falling back to default
lib/app/injection/injection.dart (2)
225-226: Consider enhancing EmailSignupCubit configuration.While the factory registration is appropriate for the signup flow, consider adding error handling configuration. Other cubits in the file have more comprehensive setups with multiple dependencies.
Example enhancement:
..registerFactory<EmailSignupCubit>( - () => EmailSignupCubit(getIt()), + () => EmailSignupCubit( + getIt(), + errorHandler: getIt<ErrorHandler>(), + networkInfo: getIt<NetworkInfo>(), + ), );
Line range hint
1-226: Consider splitting dependency injection by feature.The injection.dart file has grown significantly with many dependencies. Consider splitting it into feature-specific injection modules (e.g., auth_injection.dart, payment_injection.dart) to improve maintainability and clarity.
This would:
- Make the dependency graph clearer
- Allow for better feature isolation
- Make it easier to maintain and modify feature-specific dependencies
lib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dart (1)
Line range hint
53-53: Consider these improvements for better maintainability and reliability.
- The static
_isShowingBoxOriginflag could lead to race conditions in edge cases. Consider using state management instead.- Consider extracting animation durations into named constants.
- Use the defined static constants (homeIndex, familyIndex, profileIndex) instead of magic numbers in the array access.
Here's a suggested refactor:
- static bool _isShowingBoxOrigin = false; + // Add to state management instead + // Extract animation constants + static const Duration kAnimationDuration = Duration(milliseconds: 300); Scaffold _regularLayout({NavigationBarHomeScreenUIModel? uiModel}) { return Scaffold( // ... body: AnimatedSwitcher( - duration: const Duration(milliseconds: 300), + duration: kAnimationDuration, child: AnimatedSwitcher( - duration: const Duration(milliseconds: 300), + duration: kAnimationDuration, // ... child: <Widget>[ const FamilyHomeScreen(), const FamilyOverviewPage(), const USPersonalInfoEditPage(), - ][_currentIndex], + ][_currentIndex == NavigationBarHomeScreen.homeIndex + ? NavigationBarHomeScreen.homeIndex + : _currentIndex == NavigationBarHomeScreen.familyIndex + ? NavigationBarHomeScreen.familyIndex + : NavigationBarHomeScreen.profileIndex], ), ), ); }Also applies to: 144-233
lib/features/registration/bloc/registration_bloc.dart (2)
Line range hint
213-241: Consider improving the Stripe account verification polling mechanism.The current implementation has several potential issues:
- Hard-coded magic numbers (257 trials, 16 trials threshold)
- Exponential delay not properly implemented (only changes once)
- No timeout mechanism other than max trials
- Blocking operation that could affect UI responsiveness
Consider implementing a more robust polling mechanism:
Future<void> _onStripeSuccess( RegistrationStripeSuccess event, Emitter<RegistrationState> emit, ) async { var user = authCubit.state.user; - var trials = 1; - var delayTime = 5; + const maxAttempts = 30; + const initialDelay = Duration(seconds: 5); + const maxDelay = Duration(minutes: 2); + var currentDelay = initialDelay; + var attempts = 0; - 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: ${currentDelay.inSeconds}s, user is temporary: ${user.tempUser}'); - if (trials > 16) { - delayTime = 60; + if (!user.tempUser) break; + + attempts++; + await Future<void>.delayed(currentDelay); + + // Exponential backoff with max delay + if (currentDelay < maxDelay) { + currentDelay *= 2; + if (currentDelay > maxDelay) { + currentDelay = maxDelay; + } } - trials++; - await Future<void>.delayed(Duration(seconds: delayTime)); } if (user.tempUser == false) {
Line range hint
1-300: Consider further architectural improvements for EU/US flow separation.While the removal of
registrationRepositoryis a good start, the class still handles complex conditional logic for EU vs US flows. Consider further improvements:
Split the registration flows into separate classes:
EURegistrationBlocUSRegistrationBloc- Common base class for shared logic
Move payment-specific logic (Stripe, SEPA, BACS) to dedicated classes:
StripeRegistrationHandlerSepaRegistrationHandlerBacsRegistrationHandlerThis would align better with the PR's goal of "clearly dividing responsibilities" and make the code more maintainable.
Would you like me to provide a detailed proposal for this architectural improvement?
lib/features/family/app/injection.dart (1)
124-132: Document registration patternsConsider adding documentation comments explaining why certain cubits are registered as singletons vs factories. This would help maintain consistency as the codebase grows.
Example:
+ // Auth-related cubits + // Singletons: Used for app-wide state management + // Factories: Used for screen-specific logic ..registerLazySingleton<FamilyAuthCubit>(lib/features/family/features/login/presentation/pages/family_login_sheet.dart (1)
54-68: Simplify form validation logic and add null safety.The current implementation could be improved for better null safety and readability.
Consider this refactoring:
bool get isEnabled { - if (formKey.currentState == null) return false; - if (formKey.currentState!.validate() == false) return false; - return emailController.text.isNotEmpty && - passwordController.text.isNotEmpty; + final formState = formKey.currentState; + return formState != null && + formState.validate() && + emailController.text.isNotEmpty && + passwordController.text.isNotEmpty; }lib/features/auth/pages/login_page.dart (3)
21-21: Add documentation for the navigate callback.Consider adding a documentation comment to explain the purpose and responsibility of this callback, especially since it's an optional parameter that affects the post-login flow.
+ /// Callback function invoked after successful login to handle navigation. + /// If not provided, the default navigation behavior will be used. final Future<void> Function(BuildContext context)? navigate;
Line range hint
89-143: Consider extracting auth state handling logic.The BlocListener contains multiple similar dialog displays for different auth states. This could be simplified by:
- Creating a mapping of auth states to their corresponding error messages
- Extracting dialog display logic into a separate method or widget
// Example approach: final authErrorMessages = { AuthStatus.failure: (locals) => ( title: locals.loginFailure, content: locals.wrongCredentials, ), AuthStatus.noInternet: (locals) => ( title: locals.noInternetConnectionTitle, content: locals.noInternet, ), // ... other mappings }; void showAuthErrorDialog(BuildContext context, AuthStatus status) { final locals = context.l10n; final message = authErrorMessages[status]?.call(locals); if (message == null) return; showDialog<void>( context: context, builder: (context) => WarningDialog( title: message.title, content: message.content, onConfirm: () => context.pop(), ), ); }
Line range hint
156-187: Extract password validation logic.The password validation logic could be moved to a separate validator class to improve maintainability and reusability.
class PasswordValidator { static String? validate(String? value, AppLocalizations locals) { if (value == null || value.isEmpty) { return locals.passwordRule; } final rules = [ (String v) => v.length >= 7, (String v) => v.contains(RegExp('[0-9]')), (String v) => v.contains(RegExp('[A-Z]')), ]; return rules.every((rule) => rule(value)) ? null : locals.passwordRule; } }lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (1)
Line range hint
235-241: Consider using a builder pattern for transaction creation.The transaction creation call has multiple parameters which could make it error-prone and harder to maintain. Consider introducing a builder pattern or data class to make this more maintainable and type-safe.
Example refactor:
class TransactionBuilder { String? userId; int? amount; bool isGratitude = false; String? orgName; String? mediumId; TransactionBuilder setUserId(String id) { userId = id; return this; } // ... other setters void validate() { assert(userId != null); assert(amount != null); assert(orgName != null); assert(mediumId != null); } } // Usage: await _give.createTransaction( TransactionBuilder() .setUserId(context.read<FamilyAuthCubit>().user!.guid) .setAmount(result) .setIsGratitude(true) .setOrgName(org.name) .setMediumId(org.namespace) .build() );lib/features/family/features/auth/data/family_auth_repository.dart (4)
17-17: Document error scenarios in login method.The change from
Future<Session>toFuture<void>improves encapsulation, but callers need to understand possible failure scenarios.Add documentation to clarify potential exceptions:
/// Authenticates a user with email and password. /// Throws: /// - [NetworkException] for connection issues /// - [AuthenticationException] for invalid credentials /// - [ValidationException] for invalid input format Future<void> login(String email, String password);
Line range hint
114-129: Handle partial authentication state on user extension fetch failure.If
_fetchUserExtensionfails after storing the session, the system could be left in an inconsistent state.Consider implementing a transaction-like behavior:
Future<void> login(String email, String password) async { + Session? session; + try { final newSession = await _apiService.login({ 'username': email, 'password': password, 'grant_type': 'password', }); - var session = Session.fromJson(newSession); - session = session.copyWith(isLoggedIn: true); - await _storeSession(session); - await _fetchUserExtension(session.userGUID); + session = Session.fromJson(newSession).copyWith(isLoggedIn: true); + await Future.wait([ + _storeSession(session), + _fetchUserExtension(session.userGUID), + ]); + } catch (e) { + if (session != null) { + await logout(); // Cleanup on partial success + } + rethrow; + } }
314-335: Log failed user updates for debugging.Silent failures in update methods could make debugging difficult.
Add logging for failed updates:
Future<bool> updateUser({ required String guid, required Map<String, dynamic> newUserExt, }) async { try { final result = await _apiService.updateUser(guid, newUserExt); await _fetchUserExtension(guid); return result; - } catch (e) { + } catch (e, stack) { + LoggingInfo.instance.error( + 'Failed to update user: $e', + methodName: stack.toString(), + ); return false; } }
Line range hint
1-375: Consider future architectural improvements.While this refactor successfully simplifies the authentication logic and improves state management, consider these future improvements:
- Add retry logic for transient failures
- Implement rate limiting for API calls
- Add caching layer for user data
- Consider using sealed classes for error handling
These improvements would further enhance the robustness and maintainability of the authentication system.
lib/features/account_details/pages/personal_info_edit_page.dart (1)
106-117: Consider extracting text styles to theme constants.While the current implementation is good, consider extracting these text styles to theme constants for better maintainability and consistency across the app. This would help maintain a uniform look and make future style updates easier.
Example:
// In your theme file class AppTextStyles { static const personalInfoHeader = TextStyle(/* your styles */); static const personalInfoSubHeader = TextStyle(/* your styles */); } // In this file Text( locals.personalPageHeader, style: AppTextStyles.personalInfoHeader, ),lib/shared/widgets/custom_navigation_drawer.dart (2)
Line range hint
234-242: Consider extracting bottom sheet propertiesThe implementation is correct, but there's repeated bottom sheet configuration across multiple places. Consider extracting
isScrollControlled: true, useSafeArea: trueinto reusable constants.// Consider adding to a constants file: class BottomSheetConfig { static const defaultProps = <String, dynamic>{ 'isScrollControlled': true, 'useSafeArea': true, }; }
Line range hint
1-339: Well-structured navigation refactorThe changes successfully simplify the navigation and authentication logic while maintaining necessary UI-specific country handling. This aligns perfectly with the PR objectives of reducing complexity and improving code organization. The consistent pattern of direct navigation and proper separation of concerns will make the code more maintainable.
lib/features/give/pages/home_page.dart (1)
Line range hint
275-280: Consider simplifying the navigation pattern.While the removal of region-specific routing aligns with the PR's objectives, the nested navigation calls (
..goNamed(...).pop()) could be simplified to improve readability and prevent potential navigation stack issues.Consider this alternative approach:
-context - ..goNamed( - Pages.registration.name, - queryParameters: { - 'email': user.email, - }, - ) - ..pop(); +context.pop(); +context.goNamed( + Pages.registration.name, + queryParameters: { + 'email': user.email, + }, +);This change:
- Makes the navigation flow more explicit
- Prevents potential race conditions in navigation
- Improves code readability
lib/features/auth/cubit/auth_cubit.dart (2)
Line range hint
208-208: Address TODO comment during auth refactorSince this PR focuses on auth refactoring, this would be the ideal time to address the TODO comment. The comment lacks specific details about what aspects of the logout flow need redesign.
Would you like me to:
- Help document the specific aspects of the logout flow that need redesign?
- Create a GitHub issue to track this technical debt?
Line range hint
1-489: Consider splitting AuthCubit into focused componentsThe
AuthCubitcurrently handles multiple responsibilities including authentication, notifications, analytics, biometrics, and user preferences. This makes the class complex and harder to maintain. Consider splitting it into focused components:
AuthCubit: Core authentication logicNotificationsCubit: Firebase token managementAnalyticsCubit: Analytics trackingBiometricsCubit: Biometric checksUserPreferencesCubit: User preferencesThis would align with the PR's goal of improving separation across domain, data, and presentation layers.
lib/features/give/widgets/choose_amount.dart (1)
Line range hint
1-577: Consider these improvements for better maintainability.While the overall implementation is solid, consider these suggestions to enhance maintainability:
- Extract currency formatting logic into a dedicated service to improve reusability and testability.
- Consider using a
List<CollectionField>model class to encapsulate the field state, reducing the need for parallel lists (controllers,focusNodes,collectionFields).- Move the numeric keyboard logic into a separate widget to improve modularity.
Example of the
CollectionFieldmodel:class CollectionField { CollectionField({ required this.controller, required this.focusNode, required this.isVisible, }); final TextEditingController controller; final FocusNode focusNode; bool isVisible; void dispose() { controller.dispose(); focusNode.dispose(); } }lib/features/give/pages/organization_list_page.dart (1)
Outstanding Usage of
isUsUserDetectedMultiple instances of
isUsUserwere found in the codebase that still rely on region-specific authentication logic:
lib/utils/analytics_helper.dartlib/features/give/pages/home_page.dartlib/app/routes/app_router.dartPlease refactor these areas to ensure consistency with the authentication simplification objectives.
🔗 Analysis chain
Line range hint
383-389: LGTM: Auth simplification aligns with refactor goalsThe removal of the
isUSUserparameter from theCheckAuthRequestcallback simplifies the authentication logic, which aligns well with the PR's objective of reducing complexity in region-specific authentication handling.Let's verify that this simplification is consistently applied across the codebase:
Further verification needed: Executing additional searches for
isUSUser.
Further verification needed: Searching for all usages of
isUsUserin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of isUSUser in CheckAuthRequest rg "CheckAuthRequest.*isUSUser" # Search for other potential region-specific auth checks rg "isUSUser.*auth"Length of output: 55
Script:
#!/bin/bash # Search for any instances of isUSUser with different patterns and case variations rg -i "CheckAuthRequest.*isUSUser" rg -i "isUSUser\s*[:=]" rg -i "is_us_user" # Search for any functions or variables related to region-specific auth rg -i "region.*auth"Length of output: 191
Script:
#!/bin/bash # Search for all usages of isUsUser excluding its definition rg -i "isUsUser\b" --type dart --invert-match "bool get isUsUser" # Alternatively, search for usages in parameters and method calls rg -i "\bisUsUser\b" --type dartLength of output: 867
lib/features/family/app/family_routes.dart (3)
71-81: Add input validation for email parameterWhile the route implementation is clean, consider adding validation for the email parameter to handle edge cases.
builder: (context, state) { - final email = state.uri.queryParameters['email'] ?? ''; + final rawEmail = state.uri.queryParameters['email'] ?? ''; + final email = rawEmail.trim(); + + if (email.isNotEmpty && !RegExp(r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$').hasMatch(email)) { + log('Invalid email parameter provided: $email'); + } return UsSignUpPage( email: email, ); },
Line range hint
428-439: Extract duplicate OrganisationBloc initialization logicThis OrganisationBloc initialization pattern is duplicated from the give by list route. Consider extracting it into a reusable function.
+extension OrganisationBlocProviderX on BuildContext { + BlocProvider<OrganisationBloc> createOrganisationBlocProvider(String country) { + return BlocProvider( + create: (_) => OrganisationBloc( + getIt(), + getIt(), + )..add( + OrganisationFetch( + Country.fromCode(country), + type: CollectGroupType.none.index, + ), + ), + ); + } +} // Usage in routes: -BlocProvider( - create: (_) => OrganisationBloc( - getIt(), - getIt(), - )..add( - OrganisationFetch( - Country.fromCode(user.country), - type: CollectGroupType.none.index, - ), - ), -), +context.createOrganisationBlocProvider(user.country),
Line range hint
1-554: Consider implementing a RouteGuard pattern for authenticated routesWhile the migration to
FamilyAuthCubitimproves auth management, the current implementation requires checking user authentication in multiple places. Consider implementing a RouteGuard pattern to centralize authentication checks and reduce code duplication./// Example implementation of a route guard class AuthenticatedRoute extends GoRoute { AuthenticatedRoute({ required String path, required String name, required Widget Function(BuildContext, GoRouterState) builder, }) : super( path: path, name: name, redirect: (context, state) { final user = context.read<FamilyAuthCubit>().user; if (user == null) { // Redirect to login return '/login?redirect=${state.location}'; } return null; }, builder: builder, ); } // Usage: AuthenticatedRoute( path: FamilyPages.profileSelection.path, name: FamilyPages.profileSelection.name, builder: (context, state) { final user = context.read<FamilyAuthCubit>().user!; // Safe to use ! here // ... rest of the builder }, ),lib/app/routes/app_router.dart (1)
Line range hint
628-642: Significant improvement in authentication flow logic.The changes to
_checkAndRedirectAuthshow several improvements:
- Early return for US users, reducing complexity
- Clear separation of biometric check logic
- Improved navigation state management with proper cleanup
This aligns perfectly with the PR's objective of streamlining the authentication process and clearly dividing responsibilities.
Consider adding logging for authentication state transitions to help with debugging and monitoring in production.
static Future<void> _checkAndRedirectAuth( AuthState state, BuildContext context, GoRouterState routerState, ) async { + AnalyticsHelper.logEvent( + eventName: 'auth_state_transition', + eventProperties: { + 'status': state.status.toString(), + 'is_us_user': state.user.isUsUser, + 'has_navigation': state.hasNavigation, + }, + ); if (state.user.isUsUser) return;lib/features/family/features/login/cubit/family_login_cubit.dart (2)
35-38: Handle the case when no email is providedCurrently, when no email is provided, an error is logged but no further action is taken. Consider guiding the user appropriately in this scenario.
You might emit a specific state or navigate the user to an email input screen:
// After logging the error emitCustom(const FamilyLoginSheetCustom.requestEmailInput());This enhances user experience by providing clear guidance.
14-18: Use constants or enums for error keywordsUsing hard-coded strings for error keywords can lead to typos and inconsistencies. Consider defining constants or an enum to represent these.
Define an enum:
enum LoginErrorKeyword { twoAttemptsLeft, oneAttemptLeft, lockedOut, }Update your map:
final errorDialogs = { LoginErrorKeyword.twoAttemptsLeft: const FamilyLoginSheetCustom.showTwoAttemptsLeftDialog(), LoginErrorKeyword.oneAttemptLeft: const FamilyLoginSheetCustom.showOneAttemptLeftDialog(), LoginErrorKeyword.lockedOut: const FamilyLoginSheetCustom.showLockedOutDialog(), };And adjust the matching logic accordingly.
lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (1)
38-38: Initialize_amountdirectly to simplify codeYou can initialize
_amountat the point of declaration to make the code cleaner and eliminate the need forinitState().Apply this diff:
-final initialamount = 25; -late int _amount; +int _amount = 25;And remove the
initState()method if it contains no other logic.lib/features/email_signup/cubit/email_signup_cubit.dart (2)
132-137: Simplify thevalidateEmailmethodThe variable
resultis unnecessary. You can return the boolean expression directly for clarity and brevity.Suggested change:
bool validateEmail(String? email) { - var result = - email != null && !email.isEmpty && Util.emailRegEx.hasMatch(email); - return result; + return email != null && email.isNotEmpty && Util.emailRegEx.hasMatch(email); }
178-183: Avoid magic numbers by defining constants foramountLimitHard-coded values like
4999and499can make the code less maintainable. Define them as constants with descriptive names to improve readability and facilitate future changes.Suggested change:
// Define constants at the beginning of the class or in a separate constants file static const int usAmountLimit = 4999; static const int nonUsAmountLimit = 499; // Update the code amountLimit: _currentCountry.isUS ? usAmountLimit : nonUsAmountLimit,lib/features/family/features/registration/pages/us_signup_page.dart (4)
58-62: Consider initializing the Cubit ininitState()instead ofdidChangeDependencies()The
_cubit.init()method is called withindidChangeDependencies(), which is invoked every time the dependencies change. If the initialization is only needed once when the widget is first created, it would be more appropriate to place this logic insideinitState()to prevent unnecessary re-initialization.@override -void didChangeDependencies() { - super.didChangeDependencies(); +void initState() { + super.initState(); _cubit.init(); }
Line range hint
197-199: Avoid force unwrapping nullable valuesIn the
onTapcallback ofAcceptPolicyRowUs, you're force unwrappingvalueusing!. This could lead to a runtime error ifvalueis null.Update the code to handle potential null values safely:
onTap: (value) { setState(() { - _acceptPolicy = value!; + _acceptPolicy = value ?? false; }); },
Line range hint
225-234: Provide meaningful validation error messages and display themThe validators for the first name, last name, and password fields return empty strings when validation fails, and the
errorStylehides error messages by setting the height and font size to zero. This prevents users from receiving feedback on why their input is invalid.Update the validators to return meaningful error messages and modify the
errorStyleto display them:errorStyle: const TextStyle( - height: 0, - fontSize: 0, + color: Colors.red, ),Example of updating the first name validator:
validator: (value) { if (value == null || value.isEmpty) { - return ''; + return AppLocalizations.of(context).firstNameRequired; } if (!Util.nameFieldsRegEx.hasMatch(value)) { - return ''; + return AppLocalizations.of(context).firstNameInvalid; } return null; },Ensure that the corresponding error messages (
firstNameRequired,firstNameInvalid, etc.) are added to your localization files.Also applies to: 242-251, 284-296
Line range hint
284-313: Enhance password validation feedbackThe password validator checks for several conditions but returns generic empty strings on failure. Providing specific feedback can help users create a valid password more easily.
Update the password validator to return specific error messages:
validator: (value) { if (value == null || value.isEmpty) { - return ''; + return AppLocalizations.of(context).passwordRequired; } if (value.length < 7) { - return ''; + return AppLocalizations.of(context).passwordTooShort; } if (!value.contains(RegExp('[0-9]'))) { - return ''; + return AppLocalizations.of(context).passwordDigitRequired; } if (!value.contains(RegExp('[A-Z]'))) { - return ''; + return AppLocalizations.of(context).passwordUppercaseRequired; } return null; },Add the corresponding error messages to your localization files.
lib/features/email_signup/presentation/pages/email_signup_page.dart (2)
56-61: Refactor: Move Cubit Initialization toinitState()The
_cubit.init()method is currently called insidedidChangeDependencies(). Unless the cubit relies on inherited widgets that might change during the widget's lifecycle, it's more appropriate to initialize it ininitState(). This ensures the initialization runs only once when the widget is first inserted into the widget tree.Apply this diff to move the cubit initialization to
initState():class _EmailSignupPageState extends State<EmailSignupPage> { final _formKey = GlobalKey<FormState>(); bool _isLoading = false; final _cubit = getIt<EmailSignupCubit>(); + @override + void initState() { + super.initState(); + _cubit.init(); + _checkAuthentication(); + } - @override - void didChangeDependencies() { - super.didChangeDependencies(); - - _cubit.init(); - } - @override - void initState() { - super.initState(); - _checkAuthentication(); - }
193-219: Ensure Loading Indicator Stops After Async OperationsIn the
onTaphandler for the continue button, the loading state is set totrueusingsetLoading(), but it isn't reset tofalseafter the asynchronous operations complete. This may cause the loading indicator to persist even after the operation finishes.Apply this diff to reset the loading state after the operations:
onTap: state.continueButtonEnabled ? () async { if (state.country.isUS) { - await _cubit.login(); + setLoading(); + await _cubit.login(); + setLoading(state: false); } else { setLoading(); AppThemeSwitcher.of(context) .switchTheme(isFamilyApp: false); await context.read<AuthCubit>().register( country: state.country, email: state.email, locale: Localizations.localeOf(context) .languageCode, ); + setLoading(state: false); } } : null,lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart (5)
33-40: Conversion toStatefulWidgetmay be unnecessaryThe
USPersonalInfoEditPagehas been converted from aStatelessWidgetto aStatefulWidget, but there is no state being managed within_USPersonalInfoEditPageStateother than initializing_authCubit. If the state of the widget does not change over its lifecycle, consider keeping it as aStatelessWidgetfor simplicity.Alternatively, if you need to maintain the
_authCubitinstance, you can useBlocBuilderwithout converting the widget toStatefulWidget.
43-44: Consider lazy initialization of_authCubitThe
_authCubitis being eagerly initialized:final FamilyAuthCubit _authCubit = getIt<FamilyAuthCubit>();If
_authCubitisn't used immediately, consider lazy initialization to improve performance:late final FamilyAuthCubit _authCubit; @override void initState() { super.initState(); _authCubit = getIt<FamilyAuthCubit>(); }
209-268: Handle platform-specific biometric availability gracefullyThe code assumes that
data[0]corresponds to fingerprint availability anddata[1]to face ID. Ensure that this mapping is consistent across platforms and handle cases where biometric authentication is not available.Consider adding checks or comments to clarify the mapping and handle platforms where neither biometric option is available.
316-316: Localize the 'Logout' textThe text
'Logout'is hardcoded and not localized. To support internationalization, use the localized string fromlocals.Apply this diff:
- value: 'Logout', + value: locals.logout,Ensure that the
localsfile contains the appropriate translation for'Logout'.
321-328: Extract support email text to localization filesThe message about changing the user's name is hardcoded and may not be localized.
Extract the text to the localization files to support internationalization.
- 'Would you like to change your name? Send an e-mail to [email protected]', + locals.changeNameSupportMessage,Ensure that
locals.changeNameSupportMessageis defined in your localization files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (80)
.ruby-version(0 hunks)lib/app/app.dart(2 hunks)lib/app/injection/injection.dart(2 hunks)lib/app/routes/app_router.dart(5 hunks)lib/core/network/expired_token_retry_policy.dart(1 hunks)lib/core/network/family_expired_token_retry_policy.dart(1 hunks)lib/core/network/request_helper.dart(5 hunks)lib/core/network/token_interceptor.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/auth/cubit/auth_cubit.dart(1 hunks)lib/features/auth/cubit/auth_state.dart(2 hunks)lib/features/auth/pages/email_signup_page.dart(0 hunks)lib/features/auth/pages/login_page.dart(1 hunks)lib/features/children/add_member/pages/family_member_form_page.dart(1 hunks)lib/features/children/add_member/widgets/add_member_loading_page.dart(1 hunks)lib/features/children/details/pages/child_details_page.dart(1 hunks)lib/features/email_signup/cubit/email_signup_cubit.dart(1 hunks)lib/features/email_signup/cubit/email_signup_custom.dart(1 hunks)lib/features/email_signup/presentation/models/email_signup_uimodel.dart(1 hunks)lib/features/email_signup/presentation/pages/email_signup_page.dart(1 hunks)lib/features/family/app/family_pages.dart(1 hunks)lib/features/family/app/family_routes.dart(7 hunks)lib/features/family/app/injection.dart(6 hunks)lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart(3 hunks)lib/features/family/features/auth/bloc/family_auth_cubit.dart(1 hunks)lib/features/family/features/auth/data/family_auth_repository.dart(12 hunks)lib/features/family/features/auth/pages/family_login_page.dart(0 hunks)lib/features/family/features/avatars/screens/parent_avatar_selection_screen.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(1 hunks)lib/features/family/features/home_screen/presentation/models/navigation_bar_home_custom.dart(0 hunks)lib/features/family/features/home_screen/presentation/pages/family_home_screen.dart(2 hunks)lib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dart(2 hunks)lib/features/family/features/login/cubit/family_login_cubit.dart(1 hunks)lib/features/family/features/login/presentation/models/family_login_sheet_custom.dart(1 hunks)lib/features/family/features/login/presentation/pages/family_login_sheet.dart(1 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart(2 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart(4 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart(3 hunks)lib/features/family/features/profiles/cubit/profiles_cubit.dart(3 hunks)lib/features/family/features/profiles/repository/profiles_repository.dart(3 hunks)lib/features/family/features/profiles/widgets/profiles_empty_state_widget.dart(0 hunks)lib/features/family/features/reflect/presentation/pages/grateful_screen.dart(3 hunks)lib/features/family/features/registration/cubit/us_signup_cubit.dart(1 hunks)lib/features/family/features/registration/cubit/us_signup_custom.dart(1 hunks)lib/features/family/features/registration/pages/us_signup_page.dart(4 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/reset_password/presentation/pages/reset_password_sheet.dart(2 hunks)lib/features/family/features/topup/screens/empty_wallet_bottom_sheet.dart(3 hunks)lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart(2 hunks)lib/features/family/features/topup/screens/topup_initial_bottom_sheet.dart(2 hunks)lib/features/family/helpers/logout_helper.dart(2 hunks)lib/features/family/utils/family_auth_utils.dart(3 hunks)lib/features/give/pages/home_page.dart(3 hunks)lib/features/give/pages/organization_list_page.dart(2 hunks)lib/features/give/widgets/choose_amount.dart(1 hunks)lib/features/registration/bloc/registration_bloc.dart(1 hunks)lib/features/registration/domain/registration_repository.dart(0 hunks)lib/features/splash/cubit/splash_cubit.dart(1 hunks)lib/features/splash/cubit/splash_custom.dart(1 hunks)lib/features/splash/pages/splash_page.dart(1 hunks)lib/features/unregister_account/unregister_page.dart(1 hunks)lib/shared/models/temp_user.dart(1 hunks)lib/shared/pages/pages.dart(0 hunks)lib/shared/pages/redirect_to_browser_page.dart(2 hunks)lib/shared/pages/splash_screen.dart(0 hunks)lib/shared/widgets/buttons/leading_back_button.dart(1 hunks)lib/shared/widgets/custom_navigation_drawer.dart(8 hunks)lib/shared/widgets/outlined_text_form_field.dart(4 hunks)lib/utils/auth_utils.dart(2 hunks)lib/utils/stripe_helper.dart(1 hunks)lib/utils/util.dart(1 hunks)
💤 Files with no reviewable changes (11)
- .ruby-version
- lib/core/network/token_interceptor.dart
- lib/features/account_details/cubit/edit_stripe_cubit.dart
- lib/features/account_details/cubit/edit_stripe_state.dart
- lib/features/auth/pages/email_signup_page.dart
- lib/features/family/features/auth/pages/family_login_page.dart
- lib/features/family/features/home_screen/presentation/models/navigation_bar_home_custom.dart
- lib/features/family/features/profiles/widgets/profiles_empty_state_widget.dart
- lib/features/registration/domain/registration_repository.dart
- lib/shared/pages/pages.dart
- lib/shared/pages/splash_screen.dart
✅ Files skipped from review due to trivial changes (6)
- 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/features/registration/widgets/widgets.dart
- lib/shared/widgets/buttons/leading_back_button.dart
- lib/utils/util.dart
🧰 Additional context used
📓 Learnings (3)
lib/features/family/app/injection.dart (1)
Learnt from: Daniela510
PR: givtnl/givt-app#1239
File: lib/features/family/app/injection.dart:64-64
Timestamp: 2024-11-12T12:54:39.739Z
Learning: In `lib/features/family/app/injection.dart`, the `SetupBedtimeCubit` does not require `EditChildRepository` as a dependency and can be instantiated with just `getIt()`.
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-11-12T03:51:15.015Z
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-11-12T03:51:15.015Z
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-11-12T03:51:15.016Z
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-11-12T03:51:15.016Z
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.
🔇 Additional comments (93)
lib/features/family/features/registration/cubit/us_signup_custom.dart (1)
1-9: Verify the integration with UsSignupCubit
Since this is part of the authentication refactor (KIDS-1412), we should verify how this class is being used in the UsSignupCubit.
lib/features/email_signup/presentation/models/email_signup_uimodel.dart (1)
1-1: LGTM! Clean import statement.
The import is focused and necessary for the Country enum type.
lib/shared/pages/redirect_to_browser_page.dart (1)
3-3: LGTM! Clean import replacement
The import change aligns well with the feature-based structure and the auth refactoring objectives.
lib/features/splash/cubit/splash_custom.dart (3)
12-14: LGTM!
Clean implementation following Dart best practices with immutability.
21-23: LGTM!
Clean implementation following Dart best practices with immutability.
25-27: Verify EU/US requirements for family member addition.
Given the PR's context about differing EU/US requirements, please verify if this state needs to handle any region-specific logic for family member addition.
✅ Verification successful
EU/US Requirements Verified for Family Member Addition.
No region-specific logic is required for family member addition in SplashRedirectToAddMembers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any EU/US specific conditions related to family member addition
rg -l 'EU|US|region|country' --type dart
# Search for any existing family member related logic
ast-grep --pattern 'class $_ {
$$$
family|member|addMember
$$$
}'
Length of output: 6049
lib/features/family/features/registration/widgets/registration_app_bar.dart (1)
33-34: LGTM!
The preferredSize implementation correctly follows Flutter conventions by using the standard kToolbarHeight.
lib/core/network/expired_token_retry_policy.dart (1)
24-33:
Improve error handling in token refresh
The current error handling is too broad and may mask important issues:
- Catches all exceptions but only logs for network issues
- Silent failures could hide serious problems
- No error propagation to caller
Consider this more comprehensive approach:
- Future<void> _refreshToken() async {
+ Future<bool> _refreshToken() async {
try {
await getIt<AuthRepository>().refreshToken();
+ return true;
} catch (e) {
- log('No internet connection');
+ if (e is SocketException) {
+ log('Token refresh failed: No internet connection', error: e);
+ } else if (e is AuthException) {
+ log('Token refresh failed: Authentication error', error: e);
+ } else {
+ log('Token refresh failed: Unexpected error', error: e);
+ }
+ return false;
}
}Let's verify the AuthRepository implementation:
lib/utils/stripe_helper.dart (1)
4-4: 💡 Codebase verification
Incomplete Stripe Refactoring in Registration Domain
Remaining Stripe references found in the registration domain:
- lib/features/registration/bloc/registration_bloc.dart
- lib/features/registration/bloc/registration_state.dart
- lib/features/registration/bloc/registration_event.dart
🔗 Analysis chain
Verify the new location of StripeCubit
The import path change from registration to family domain aligns with the PR's goal of better separation of concerns. However, let's verify that all related Stripe functionality is consistently placed under the family domain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Stripe-related code under the registration domain
# and verify consistent placement under family domain
echo "Checking for any remaining Stripe references in registration domain..."
rg -l "stripe|Stripe" "lib/features/registration"
echo "Verifying consistent placement under family domain..."
rg -l "stripe|Stripe" "lib/features/family"
Length of output: 830
⛔ Skipped due to learnings
Learnt from: Daniela510
PR: givtnl/givt-app#1149
File: lib/features/family/app/family_routes.dart:98-98
Timestamp: 2024-11-12T03:51:15.016Z
Learning: In `lib/features/family/app/family_routes.dart` and related files, `StripeCubit` is provided via `getIt` using dependency injection, and pages access it through `getIt` instead of direct provision via `MultiBlocProvider`.
Learnt from: Daniela510
PR: givtnl/givt-app#1130
File: lib/features/family/app/family_routes.dart:100-103
Timestamp: 2024-11-12T03:51:15.015Z
Learning: In `lib/features/family/app/family_routes.dart`, both instances of `StripeCubit` are intentionally provided to `NavigationBarHomeScreen` because they serve specific purposes.
Learnt from: Daniela510
PR: givtnl/givt-app#1130
File: lib/features/family/app/family_routes.dart:100-103
Timestamp: 2024-11-12T03:51:15.016Z
Learning: In `lib/features/family/app/family_routes.dart`, both instances of `StripeCubit` are intentionally provided to `NavigationBarHomeScreen` because they serve specific purposes.
lib/core/network/family_expired_token_retry_policy.dart (1)
1-39: Verify integration with RequestHelper
The implementation looks solid, but let's verify its integration with RequestHelper to ensure it's being used correctly.
✅ Verification successful
Verified integration with RequestHelper
The FamilyExpiredTokenRetryPolicy is correctly integrated with RequestHelper, and no conflicting retry policies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RequestHelper usage of this policy
rg -l "FamilyExpiredTokenRetryPolicy" | grep -v "family_expired_token_retry_policy.dart"
# Check for any other retry policies that might conflict
ast-grep --pattern 'class $_ extends RetryPolicy'
Length of output: 175
lib/features/splash/cubit/splash_cubit.dart (3)
1-7: LGTM! Clean imports and proper class declaration.
The imports are well-organized and the class properly extends CommonCubit with appropriate type parameters.
15-16: LGTM! Well-defined repository fields.
Fields are properly encapsulated and immutable.
18-39:
Add error handling and improve state management.
The initialization flow has several potential issues that should be addressed:
- Missing error handling for async operations
- No loading state updates during async operations
- Possible race condition between user and profile checks
- Missing null check for profiles
Consider applying these improvements:
Future<void> init() async {
+ try {
+ emitLoading();
await _authRepository.initAuth();
final user = _authRepository.getCurrentUser();
+ if (user == null) {
+ emitCustom(const SplashCustom.redirectToWelcome());
+ return;
+ }
+
+ if (!user.personalInfoRegistered) {
+ emitCustom(SplashCustom.redirectToSignup(user.email));
+ return;
+ }
+
final profiles = await _profilesRepository.refreshProfiles();
-
- if (user == null) {
- emitCustom(const SplashCustom.redirectToWelcome());
- return;
- }
-
- if (!user.personalInfoRegistered) {
- emitCustom(SplashCustom.redirectToSignup(user.email));
- return;
+ if (profiles == null) {
+ emitError('Failed to load profiles');
+ return;
}
if (profiles.length <= 1) {
emitCustom(const SplashCustom.redirectToAddMembers());
return;
}
emitCustom(const SplashCustom.redirectToHome());
+ } catch (e) {
+ emitError('Failed to initialize: $e');
}
}Let's verify the error handling in other cubits:
lib/features/family/features/login/presentation/models/family_login_sheet_custom.dart (1)
31-49: Consider adding state-specific data and behavior.
The concrete implementations are clean but minimal. Consider whether they should carry additional data or behavior specific to each state:
- Error messages or localization keys
- Retry timeouts for locked state
- Navigation paths for success state
Let's verify if similar dialog states exist elsewhere in the codebase:
lib/features/family/features/registration/widgets/registered_check_animation.dart (2)
5-13: LGTM!
The widget declaration follows Flutter best practices with a const constructor and proper override implementation.
35-35: Verify the existence of referenced widgets
The code references registeredCheck and registeredCheckBackground widgets which are not defined in the current file. Please ensure these widgets are properly imported and available.
Also applies to: 39-42
lib/features/family/helpers/logout_helper.dart (1)
6-7: Verify the coexistence of AuthCubit and FamilyAuthRepository
The presence of both AuthCubit and FamilyAuthRepository imports suggests that both authentication systems are active. Is this intentional or a transitional state?
lib/features/children/add_member/widgets/add_member_loading_page.dart (1)
9-9: LGTM: Import aligns with navigation refactor
The addition of go_router import is appropriate for the new navigation implementation.
lib/features/family/features/auth/bloc/family_auth_cubit.dart (2)
56-58:
Review async behavior of initAuth
The checkAuthOnAppStartup method appears to make a synchronous call to what might be an async operation. This could lead to race conditions or missed errors.
Let's verify the repository implementation:
Consider making the method async and adding error handling:
-void checkAuthOnAppStartup() {
- _authRepository.initAuth();
+Future<void> checkAuthOnAppStartup() async {
+ try {
+ await _authRepository.initAuth();
+ } catch (e) {
+ emit(FamilyAuthState.error(e.toString()));
+ }37-42: Document migration path for legacy getter
While the implementation is clean, the "Do not use" comment should be enhanced to guide developers on the preferred approach for new code.
Let's verify the current usage to track the technical debt:
lib/features/auth/cubit/auth_state.dart (3)
40-42: LGTM: Empty navigation handler updated consistently
The _emptyNavigate static method has been correctly updated to match the new navigation signature.
55-55: LGTM: CopyWith method parameter aligned with new type
The copyWith method's navigate parameter type has been properly updated to maintain consistency with the simplified navigation signature.
38-38: LGTM: Simplified navigation signature aligns with refactor goals
The removal of the isUSUser parameter from the navigation function signature supports the PR's objective of reducing region-specific complexity in the authentication flow.
Let's verify that this change is consistently applied across the codebase:
✅ Verification successful
Verified: isUSUser parameter successfully removed from navigation-related code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining isUSUser parameters in navigation-related code
# Expected: No occurrences of isUSUser in navigation contexts
# Search for isUSUser in navigation-related code
rg "isUSUser.*navigate"
# Search for potential navigation-related files that might need updates
ast-grep --pattern 'navigate.*isUSUser'
Length of output: 67
Script:
#!/bin/bash
# Description: Comprehensive search for any remaining isUSUser usages in the codebase
# Search for any occurrence of isUSUser in the repository
rg "isUSUser"
# Specifically search for isUSUser in function signatures related to navigation
rg "Function.*isUSUser.*navigate"
# Use ast-grep to find function definitions that include isUSUser and navigate
ast-grep --pattern 'function $_($args) { $$$ isUSUser $$$ navigate $$$ }'
Length of output: 125
lib/features/family/features/registration/widgets/accept_policy_row_us.dart (1)
1-7: LGTM! Imports are clean and purposeful.
All imports are relevant to the widget's functionality.
lib/features/family/features/topup/screens/enter_details_bottom_sheet.dart (1)
31-45: LGTM! Clean navigation implementation.
The navigation flow is well-structured with proper error handling and callback usage. The analytics event tracking is a good addition for monitoring user interactions.
Let's verify the navigation flow implementation:
✅ Verification successful
Verified! The navigation flow and analytics implementation are correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the navigation implementation across related files
# Check for any forced navigation or double screen issues
rg -A 5 "Navigator.of\(context\).pop|pushNamed|pushReplacementNamed" lib/features/family/
# Verify the analytics event is properly defined
rg "enterCardDetailsClicked" lib/core/enums/amplitude_events.dart
# Check TopupWalletBottomSheet show method implementation
ast-grep --pattern 'class TopupWalletBottomSheet {
$$$
static void show($$$) {
$$$
}
$$$
}'
Length of output: 25710
lib/features/splash/pages/splash_page.dart (1)
1-19: LGTM! Clean imports and proper widget declaration.
The imports are well-organized and the StatefulWidget declaration follows Flutter best practices.
lib/shared/widgets/outlined_text_form_field.dart (1)
Line range hint 9-72: Overall changes improve widget flexibility
The modifications to make the controller optional and support initial values align well with the authentication refactor objectives. The changes maintain backward compatibility while adding useful functionality for different form scenarios.
lib/features/family/features/registration/cubit/us_signup_cubit.dart (4)
17-17: LGTM!
The field declaration follows best practices with proper encapsulation and immutability.
4-4: Verify the import filename.
The import us_signup_custom.dart seems unusual. Should this be us_signup_custom_state.dart instead? This appears to be the state file for this cubit based on the usage of UsSignupCustomSuccess on line 70.
#!/bin/bash
# Search for the correct filename
fd -e dart "us_signup_custom" 52-61: Review the use of default values.
The code uses several default values from the Util class. This could be problematic:
- Default values might not be appropriate for all cases
- It's unclear why these fields need default values
- Some fields like IBAN and postal code might have country-specific formats
#!/bin/bash
# Check the default values in Util class
rg "defaultAddress|defaultCity|defaultPostCode|defaultIban" -A 265-68: Clarify the hardcoded isNewUser parameter.
The isNewUser parameter is hardcoded to false. This seems counterintuitive for a signup process.
lib/features/family/utils/family_auth_utils.dart (3)
7-7: LGTM! Import updated to reflect new UI component.
The import change aligns with the shift from FamilyLoginPage to FamilyLoginSheet component.
73-73: Verify FamilyLoginSheet integration.
The changes look good, but let's verify the FamilyLoginSheet component accepts these props.
Also applies to: 79-81
93-102: Verify the removal of isUSUser parameter impact.
The removal of isUSUser from the navigate callback suggests a more unified approach. Let's verify this doesn't break existing navigation logic.
lib/features/family/app/family_pages.dart (1)
25-25: Verify the impact of absolute path on routing behavior.
The path for registrationUS has been changed to an absolute path (with leading slash). While this change aligns with some other routes like profileSelection, ensure this doesn't affect the navigation flow in the new authentication system.
✅ Verification successful
Route path format is consistent after adding the leading slash.
The path for registrationUS aligns with other route definitions and does not affect the navigation flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for route handling consistency across the codebase
# Search for route handling logic
rg -l "GoRouter|RouterConfig|onGenerateRoute" --type dart
# Search for registrationUS route usage
rg "registrationUS" --type dart
Length of output: 606
Script:
#!/bin/bash
# Description: Extract and analyze route definitions for path format consistency
# Extract all route paths from app_router.dart
rg 'path:\s*["\']/[^"\']+["\']' lib/app/routes/app_router.dart
# List all routes to check for leading slashes
rg 'path:\s*["\']/[^"\']*["\']' lib/app/routes/app_router.dart
Length of output: 93
Script:
#!/bin/bash
# Description: Extract and analyze route definitions for path format consistency
# Extract all route paths from app_router.dart
rg 'path:\s*["\']/[^"\']+["\']' lib/app/routes/app_router.dart
# List all routes to check for leading slashes
rg 'path:\s*["\']/[^"\']*["\']' lib/app/routes/app_router.dart
Length of output: 93
Script:
#!/bin/bash
# Description: Extract and analyze route definitions for path format consistency
# Extract all route paths from app_router.dart
rg "path:\\s*['\"]/[^'\"]+['\"]" lib/app/routes/app_router.dart
# List all routes to check for leading slashes
rg "path:\\s*['\"]/[^'\"]*['\"]" lib/app/routes/app_router.dart
Length of output: 328
lib/utils/auth_utils.dart (1)
18-18: Verify all callers of navigate callback have been updated
The removal of the isUSUser parameter from the navigate callback signature aligns well with the PR's objective to simplify authentication logic. This is a breaking change that streamlines the codebase by removing EU/US conditional complexity.
lib/features/family/features/topup/screens/empty_wallet_bottom_sheet.dart (1)
5-5: LGTM: Import change aligns with auth refactoring
The replacement of AuthCubit with FamilyAuthCubit is consistent with the PR's objective to refactor the authentication system.
lib/features/family/features/topup/screens/topup_initial_bottom_sheet.dart (1)
6-6: LGTM! Import change aligns with auth refactoring goals.
The updated import reflects the new family-specific authentication structure, improving domain separation.
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (1)
10-10: LGTM: Clean transition to FamilyAuthCubit
The import change aligns well with the PR objectives of refactoring authentication and simplifying the codebase.
lib/features/family/features/registration/widgets/mobile_number_form_field.dart (1)
1-6: LGTM! Clean imports and well-defined type.
The imports are appropriate and the typedef provides good type safety for the callback functions.
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 as a StatelessWidget with the required id parameter.
lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart (2)
102-105: LGTM! Good improvement in data emission logic.
The addition of the profiles list check before emitting data is a good safety measure, and removing redundant profile fetching helps simplify the code.
Line range hint 19-23: Verify RegistrationUseCase dependency after repository removal
The class still mixes in RegistrationUseCase but RegistrationRepository was removed from the constructor. This could lead to runtime errors if RegistrationUseCase depends on RegistrationRepository.
lib/features/family/features/profiles/cubit/profiles_cubit.dart (2)
5-5: LGTM: Repository dependency updates align with auth refactor goals
The change from AuthRepository to FamilyAuthRepository is consistent with the PR's objective to simplify authentication logic and improve separation of concerns.
Also applies to: 25-25
160-160: LGTM: Proper resource cleanup
The subscription cleanup in the close method ensures proper resource management.
lib/features/family/features/avatars/screens/parent_avatar_selection_screen.dart (1)
5-5: LGTM! Import change aligns with architecture goals.
The updated import path properly reflects the new organization of authentication logic under the family feature domain, supporting better separation of concerns.
lib/features/unregister_account/unregister_page.dart (2)
4-4: LGTM: Import path update aligns with the new structure.
The simplified import path for logout_helper reflects the structural reorganization mentioned in the PR objectives, making the helpers more accessible at the feature level.
Line range hint 67-67: Verify logout helper implementation for account termination.
The logout helper is called with fromTerminateAccount: true, which seems to be a special flag for account termination scenarios.
Let's verify the implementation of this flag:
#!/bin/bash
# Check the implementation of logout helper
ast-grep --pattern 'logout($$$fromTerminateAccount$$$) {
$$$
}'
# Check other usages of this flag
rg "fromTerminateAccount" -A 3lib/features/family/features/creditcard_setup/pages/credit_card_details.dart (1)
Line range hint 94-94: Verify NavigationBarHomeCubit.refreshData() method existence
The code calls refreshData() on the NavigationBarHomeCubit. Let's verify this method exists and is properly implemented.
✅ Verification successful
NavigationBarHomeCubit.refreshData() method exists and is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of refreshData method
# Search for refreshData method in NavigationBarHomeCubit
ast-grep --pattern 'class NavigationBarHomeCubit {
$$$
refreshData() {
$$$
}
$$$
}'
# Backup: Search using ripgrep
rg -p "refreshData.*\{" "lib/features/family/features/home_screen/cubit/"
Length of output: 289
lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (1)
10-10: LGTM: Import change aligns with auth refactoring
The change from AuthCubit to FamilyAuthCubit properly reflects the architectural changes described in the PR objectives.
lib/features/family/features/reset_password/presentation/pages/reset_password_sheet.dart (1)
16-25: LGTM! Appropriate widget naming.
The rename from ResetPasswordScreen to ResetPasswordSheet better reflects its usage as a modal bottom sheet component.
lib/app/app.dart (1)
13-13: LGTM: Import aligns with auth refactor objectives
The addition of the FamilyAuthCubit import supports the PR's goal of separating authentication responsibilities and improving code organization.
lib/features/family/features/home_screen/presentation/pages/family_home_screen.dart (2)
7-7: LGTM! Import changes align with auth refactoring goals.
The new imports for FamilyAuthCubit and its state model reflect the transition to a more domain-specific authentication system, which aligns well with the PR's objective to clearly separate responsibilities.
Also applies to: 11-11
176-177: LGTM! Auth request changes align with refactoring goals.
The transition to FamilyCheckAuthRequest and simplified navigation callback aligns with the PR's objective to reduce complexity in the authentication system.
lib/features/family/features/profiles/repository/profiles_repository.dart (1)
9-9: LGTM: Auth dependency updates align with refactor goals
The replacement of AuthRepository with FamilyAuthRepository aligns well with the PR's objective to streamline authentication and improve separation of concerns.
Also applies to: 54-54
lib/core/network/request_helper.dart (2)
12-13: LGTM! New imports support region-specific retry policies.
The addition of separate retry policies for EU and US aligns well with the PR's objective to better handle regional differences in authentication logic.
85-86: Verify the impact of split client creation.
The separation of EU and US clients is a good architectural change. However, we should verify:
- All existing code paths are updated to use the correct client
- No regression in error handling behavior
Also applies to: 95-96
✅ Verification successful
Client creation split verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct client usage to ensure all are updated
rg -l "client\." --type dart | xargs rg "client\." -B 2 -A 2
# Search for any remaining references to old createClient method
rg "createClient\(" --type dart
Length of output: 24425
Script:
#!/bin/bash
# Refined search for client usage to ensure all instances are updated
rg "createEUClient\(" --type dart
rg "createUSClient\(" --type dart
# Search for any remaining references to the old createClient method
rg "createClient\(" --type dart
Length of output: 605
lib/app/injection/injection.dart (2)
18-19: LGTM: New imports are properly organized.
The imports for EmailSignupCubit and StripeCubit are correctly placed and follow the project's import structure.
223-224: Verify EU/US handling in StripeCubit.
Given that this PR aims to address EU/US logic differences, please ensure that StripeCubit properly handles region-specific payment requirements.
Let's verify the implementation:
lib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dart (1)
13-13: LGTM! Import changes align with functionality changes.
The addition of box_origin_selection_page.dart import supports the new box origin dialog functionality, while the removal of add_member_util.dart aligns with the removal of the family setup flow.
lib/features/registration/bloc/registration_bloc.dart (1)
19-19: LGTM: Constructor change aligns with refactoring goals.
The removal of registrationRepository dependency simplifies the class's responsibilities and aligns with the PR's objective of streamlining authentication logic.
lib/features/family/app/injection.dart (2)
145-150: Document repository dependencies
The FamilyAuthRepositoryImpl is constructed with two dependencies. Consider adding a comment explaining what these dependencies are for better maintainability.
133-134: Verify SplashCubit dependencies
The SplashCubit is registered with two dependencies (getIt(), getIt()). Consider adding a comment explaining the purpose of each dependency for better maintainability.
lib/features/family/features/login/presentation/pages/family_login_sheet.dart (1)
1-30: LGTM! Clean class structure with good separation of concerns.
The class structure follows best practices with:
- Clear dependency management
- Flexible navigation through callback pattern
- Optional email parameter for different use cases
lib/features/auth/pages/login_page.dart (2)
Line range hint 1-248: Implementation looks solid overall.
The login page implementation is well-structured and handles various edge cases appropriately. The removal of region-specific logic aligns well with the PR's objective to simplify the authentication system.
21-21: Verify navigation flows after signature simplification.
The removal of isUSUser parameter aligns well with the PR's goal to reduce complexity. However, we should verify that region-specific routing is handled appropriately elsewhere in the codebase.
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (1)
7-7: LGTM: Import change aligns with auth refactor.
The import change from AuthCubit to FamilyAuthCubit is consistent with the PR's auth refactoring objectives.
lib/features/account_details/pages/personal_info_edit_page.dart (1)
104-122: LGTM! Clean implementation of the header section.
The new Column widget with header text is well-structured, properly spaced, and follows Flutter best practices. The implementation aligns well with the PR's goal of simplifying the UI logic.
lib/shared/widgets/custom_navigation_drawer.dart (7)
Line range hint 53-61: LGTM: Simplified registration navigation
The registration navigation has been streamlined by removing conditional logic and maintaining only essential parameters. This aligns well with the PR's objective to simplify authentication logic.
Line range hint 82-91: LGTM: Streamlined navigation with proper analytics
The navigation logic has been simplified while maintaining analytics tracking. The unawaited wrapper for analytics is a good practice to prevent blocking the UI thread.
104-107: LGTM: Direct navigation implementation
The history navigation has been simplified while maintaining the necessary data sync functionality.
Line range hint 118-127: LGTM: Consistent navigation pattern
The recurring donations navigation follows the same simplified pattern as other sections, with proper analytics tracking.
Line range hint 141-155: LGTM: Maintained necessary country-specific UI elements
While the auth logic has been simplified, the country-specific currency display is correctly maintained as it's essential for the UI.
164-167: LGTM: Clean navigation implementation
The personal info navigation follows the simplified pattern consistently.
286-289: LGTM: Consistent navigation pattern
The unregister navigation maintains the simplified pattern seen throughout the changes.
lib/features/give/pages/home_page.dart (2)
252-252: LGTM! Standardized dialog title.
Using a consistent "Important Reminder" title regardless of region simplifies the code and improves maintainability, aligning with the PR's goal of reducing regional differences in the authentication flow.
182-185: LGTM! Simplified registration dialog condition.
The simplified condition aligns well with the PR's objective of reducing complexity in the authentication logic. The check is now more straightforward and focuses only on essential conditions.
Let's verify that this simplification doesn't miss any edge cases:
✅ Verification successful
Verified: Simplified registration condition does not miss any edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other registration-related conditions that might need similar simplification
rg "needRegistration|mandateSigned" --type dart
Length of output: 3091
lib/features/auth/cubit/auth_cubit.dart (1)
41-41: Verify navigation handling for EU/US users
The simplified navigation callback removes the isUSUser parameter, which aligns with the PR's goal of reducing EU/US complexity. However, we need to ensure that region-specific routing is properly handled elsewhere.
Let's verify how region-specific navigation is handled:
✅ Verification successful
Navigation handling for EU/US users is adequately managed
The removal of the isUSUser parameter aligns with the refactoring goals. Region-specific routing is handled within app.dart and app_router.dart through country code checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EU/US specific navigation logic that might have replaced the removed parameter
rg -l "isUSUser|countryCode.*US|region.*EU" --type dart
# Look for navigation-related changes in router files
fd -e dart router
Length of output: 125
Script:
#!/bin/bash
# Extract navigation logic related to user regions in app.dart
ast-grep --pattern $'class App {
$$$
navigate($_) {
$$$
}
$$$
}' lib/app/app.dart
# Extract routing configurations in app_router.dart
ast-grep --pattern $'class AppRouter {
$$$
RouteRoute($_) {
$$$
}
$$$
}' lib/app/routes/app_router.dart
Length of output: 211
Script:
#!/bin/bash
# Search for navigation-related keywords in app.dart
rg -i "navigate|route|navigation|region|countryCode|EU|US" lib/app/app.dart
# Search for navigation-related keywords in app_router.dart
rg -i "navigate|route|navigation|region|countryCode|EU|US" lib/app/routes/app_router.dart
Length of output: 5064
Script:
#!/bin/bash
# Broad search for navigation and region-related logic in app.dart
rg -i "navigate|navigateTo|navigation|router|route|routes|region|country|locale|isUSUser|isEuUser|handleNavigation" lib/app/app.dart
# Broad search for navigation and region-related logic in app_router.dart
rg -i "navigate|navigateTo|navigation|router|route|routes|region|country|locale|isUSUser|isEuUser|handleNavigation" lib/app/routes/app_router.dart
Length of output: 4079
lib/features/give/widgets/choose_amount.dart (1)
Line range hint 340-352: LGTM! Auth flow simplification aligns with refactor goals.
The removal of the isUSUser parameter from the navigation callback aligns with the PR's objective of simplifying authentication logic between EU and US regions. The change maintains the core functionality while reducing complexity.
Let's verify the auth flow changes:
✅ Verification successful
LGTM! Auth flow simplification verified and no isUSUser parameters found in AuthUtils.checkToken calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the auth flow changes and usage of AuthUtils.checkToken
# Check for any remaining isUSUser parameters in auth flows
rg -l "isUSUser.*AuthUtils.checkToken"
# Check for any potential breaking changes in AuthUtils.checkToken usage
ast-grep --pattern 'AuthUtils.checkToken($$$)'
Length of output: 12095
lib/features/give/pages/organization_list_page.dart (1)
Line range hint 431-436: LGTM: Consistent auth handling in bottom sheet
The same auth simplification is correctly applied here in the modal bottom sheet implementation, maintaining consistency with the iOS-specific implementation above.
lib/features/family/app/family_routes.dart (1)
19-19: LGTM: Import aligns with auth refactor objectives
The addition of FamilyAuthCubit import supports the PR's goal of streamlining authentication responsibilities.
lib/app/routes/app_router.dart (3)
141-141: LGTM: SplashPage replacement improves consistency.
Replacing LoadingPage with SplashPage in the route definition maintains consistency with other route changes and aligns with the authentication refactor.
632-632: Verify biometric check implementation.
The biometric check is using a login-specific request. Ensure this handles all biometric authentication scenarios correctly.
#!/bin/bash
# Search for all biometric-related implementations
echo "Checking biometric implementations..."
rg "PermitBiometricRequest" -A 5
# Look for potential edge cases in biometric handling
ast-grep --pattern 'class PermitBiometricRequest {
$$$
login() {
$$$
}
$$$
}'13-13: Verify import path changes align with project structure.
The EmailSignupPage import path has been updated to follow a cleaner feature-based structure, and a new SplashPage has been added. This aligns with the PR's goal of improving separation of concerns.
Also applies to: 40-40
lib/features/family/features/login/cubit/family_login_cubit.dart (1)
28-32: Verify if getCurrentUser() is synchronous
The getCurrentUser() method is called without await. Ensure that this method is synchronous. If it's asynchronous, the init method should be modified accordingly.
Check if getCurrentUser() is asynchronous. If so, refactor the init method:
void init(String? email) async {
emitInitial();
if (email != null) {
emitData(email);
return;
}
final user = await _authRepository.getCurrentUser();
if (user != null) {
emitData(user.email);
return;
}
// Rest of the code
}lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (2)
4-6: Imports are correctly updated for family authentication
The imports for FamilyAuthCubit, FamilyAuthState, and CreditCardDetails are appropriately added to support the new family authentication flow.
97-98: Ensure navigation callback handles authentication correctly
The use of FamilyCheckAuthRequest with the navigate callback properly redirects authenticated users. The implementation aligns with the new authentication flow.
lib/features/family/features/registration/pages/us_signup_page.dart (3)
101-104: Confirm if logging out on back button press is intended
Pressing the back button triggers a logout action. This behavior might be unexpected for users who anticipate navigating to the previous screen rather than logging out entirely.
Please verify if this is the desired user experience. If the intention is to prevent users from going back during the signup process, consider disabling the back button or showing a confirmation dialog before logging out.
157-182: Duplicate parameters: country and countryCode
Both country and countryCode are being passed the same value _selectedCountry.countryCode in _cubit.savePersonalInfo. This might be redundant unless both parameters serve distinct purposes.
Please confirm if both parameters are necessary. If they represent the same data, consider removing one to simplify the function call:
await _cubit.savePersonalInfo(
email: _emailController.text,
password: _passwordController.text,
firstName: _firstNameController.text,
lastName: _lastNameController.text,
- country: _selectedCountry.countryCode,
phoneNumber: _phoneNumberController.text,
appLanguage: Localizations.localeOf(context).languageCode,
countryCode: _selectedCountry.countryCode,
profilePicture: avatar.fileName,
);129-136: Handle null or missing avatars gracefully
When fetching the avatar using user.guid, there might be cases where the avatar is null or not set. Ensure that the app handles such scenarios without crashing.
Verify that AvatarSelectionBottomsheet.show and RandomAvatar can handle null or missing avatars. If necessary, provide a default avatar or prompt the user to select one.
lib/features/family/features/account/presentation/pages/us_personal_info_edit_page.dart (3)
247-259: Verify correct usage of FamilyAuthUtils.authenticateUser
Ensure that the authenticateUser method is correctly utilized and that the navigation logic within FamilyCheckAuthRequest aligns with the intended flow.
Double-check the implementation of FamilyAuthUtils.authenticateUser to ensure it handles the navigate callback appropriately and that the biometric authentication flows as expected.
201-207: Confirm display of ResetPasswordSheet
The onTap callback for changing the password invokes ResetPasswordSheet(initialEmail: user.email).show(context). Verify that the show method properly displays the bottom sheet and that the ResetPasswordSheet is functioning as intended.
Ensure that ResetPasswordSheet's show method is correctly implemented and that it doesn't cause any navigation issues.
273-286: Ensure navigation to the unregister page is correct
In the onTap callback for unregistering, the navigation uses context.pushNamed(FamilyPages.unregisterUS.name). Verify that the route name is correct and that the navigation performs as expected.
Check that FamilyPages.unregisterUS is correctly defined and registered in your router configuration.
| sealed class UsSignupCustom { | ||
| const UsSignupCustom(); | ||
|
|
||
| const factory UsSignupCustom.succes() = UsSignupCustomSuccess; | ||
| } | ||
|
|
||
| class UsSignupCustomSuccess extends UsSignupCustom { | ||
| const UsSignupCustomSuccess(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding additional states for a complete signup flow
The current implementation only includes a success state, but a typical signup flow should handle multiple states. Consider adding:
- Loading state during API calls
- Error state for validation/network failures
- Initial/idle state
sealed class UsSignupCustom {
const UsSignupCustom();
- const factory UsSignupCustom.succes() = UsSignupCustomSuccess;
+ const factory UsSignupCustom.initial() = UsSignupCustomInitial;
+ const factory UsSignupCustom.loading() = UsSignupCustomLoading;
+ const factory UsSignupCustom.success() = UsSignupCustomSuccess;
+ const factory UsSignupCustom.error(String message) = UsSignupCustomError;
}
class UsSignupCustomSuccess extends UsSignupCustom {
const UsSignupCustomSuccess();
}
+
+class UsSignupCustomInitial extends UsSignupCustom {
+ const UsSignupCustomInitial();
+}
+
+class UsSignupCustomLoading extends UsSignupCustom {
+ const UsSignupCustomLoading();
+}
+
+class UsSignupCustomError extends UsSignupCustom {
+ final String message;
+ const UsSignupCustomError(this.message);
+}📝 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.
| sealed class UsSignupCustom { | |
| const UsSignupCustom(); | |
| const factory UsSignupCustom.succes() = UsSignupCustomSuccess; | |
| } | |
| class UsSignupCustomSuccess extends UsSignupCustom { | |
| const UsSignupCustomSuccess(); | |
| } | |
| sealed class UsSignupCustom { | |
| const UsSignupCustom(); | |
| const factory UsSignupCustom.initial() = UsSignupCustomInitial; | |
| const factory UsSignupCustom.loading() = UsSignupCustomLoading; | |
| const factory UsSignupCustom.success() = UsSignupCustomSuccess; | |
| const factory UsSignupCustom.error(String message) = UsSignupCustomError; | |
| } | |
| class UsSignupCustomSuccess extends UsSignupCustom { | |
| const UsSignupCustomSuccess(); | |
| } | |
| class UsSignupCustomInitial extends UsSignupCustom { | |
| const UsSignupCustomInitial(); | |
| } | |
| class UsSignupCustomLoading extends UsSignupCustom { | |
| const UsSignupCustomLoading(); | |
| } | |
| class UsSignupCustomError extends UsSignupCustom { | |
| final String message; | |
| const UsSignupCustomError(this.message); | |
| } |
| class EmailSignupUiModel { | ||
| const EmailSignupUiModel({ | ||
| required this.email, | ||
| required this.country, | ||
| required this.continueButtonEnabled, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add copyWith and equality methods for better state management.
While the immutable design with required parameters is good, this UI model would benefit from some essential methods for state management.
Consider adding:
class EmailSignupUiModel {
const EmailSignupUiModel({
required this.email,
required this.country,
required this.continueButtonEnabled,
});
+ EmailSignupUiModel copyWith({
+ String? email,
+ Country? country,
+ bool? continueButtonEnabled,
+ }) {
+ return EmailSignupUiModel(
+ email: email ?? this.email,
+ country: country ?? this.country,
+ continueButtonEnabled: continueButtonEnabled ?? this.continueButtonEnabled,
+ );
+ }
+
+ @override
+ bool operator ==(Object other) =>
+ identical(this, other) ||
+ other is EmailSignupUiModel &&
+ email == other.email &&
+ country == other.country &&
+ continueButtonEnabled == other.continueButtonEnabled;
+
+ @override
+ int get hashCode => Object.hash(email, country, continueButtonEnabled);Committable suggestion skipped: line range outside the PR's diff.
| launchUrl(Uri.parse(uri)).then((value) { | ||
| context.goNamed(Pages.splash.name); | ||
| }); | ||
| return const LoadingPage(); | ||
| return const SplashPage(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for URL launch failures
The current implementation doesn't handle URL launch failures, which could leave users stranded if the browser launch fails. Consider adding error handling and user feedback.
Here's a suggested implementation:
@override
Widget build(BuildContext context) {
- launchUrl(Uri.parse(uri)).then((value) {
+ launchUrl(Uri.parse(uri)).then((launched) {
+ if (!launched) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ const SnackBar(
+ content: Text('Failed to open browser. Please try again.'),
+ ),
+ );
+ }
context.goNamed(Pages.splash.name);
+ }).catchError((error) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ const SnackBar(
+ content: Text('An error occurred while opening the browser.'),
+ ),
+ );
+ context.goNamed(Pages.splash.name);
});
return const SplashPage();
}📝 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.
| launchUrl(Uri.parse(uri)).then((value) { | |
| context.goNamed(Pages.splash.name); | |
| }); | |
| return const LoadingPage(); | |
| return const SplashPage(); | |
| } | |
| launchUrl(Uri.parse(uri)).then((launched) { | |
| if (!launched) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Failed to open browser. Please try again.'), | |
| ), | |
| ); | |
| } | |
| context.goNamed(Pages.splash.name); | |
| }).catchError((error) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('An error occurred while opening the browser.'), | |
| ), | |
| ); | |
| context.goNamed(Pages.splash.name); | |
| }); | |
| return const SplashPage(); | |
| } |
| final Widget? leading; | ||
| final Text? title; | ||
| final List<Widget>? actions; | ||
| final bool isUS; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type inconsistency in actions property.
The actions property is declared as nullable List<Widget>? but has a non-null default value of const [] in the constructor. This creates unnecessary nullable handling.
- final List<Widget>? actions;
+ final List<Widget> actions;📝 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 Widget? leading; | |
| final Text? title; | |
| final List<Widget>? actions; | |
| final bool isUS; | |
| final Widget? leading; | |
| final Text? title; | |
| final List<Widget> actions; | |
| final bool isUS; |
| class EmailSignupShowFamilyRegistration extends EmailSignupCustom { | ||
| const EmailSignupShowFamilyRegistration(this.email); | ||
|
|
||
| final String email; | ||
| } | ||
|
|
||
| class EmailSignupShowFamilyLogin extends EmailSignupCustom { | ||
| const EmailSignupShowFamilyLogin(this.email); | ||
|
|
||
| final String email; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add email validation and implement equality
The email-carrying states should validate their input and implement proper equality checks.
Consider these improvements:
+import 'package:equatable/equatable.dart';
-class EmailSignupShowFamilyRegistration extends EmailSignupCustom {
+class EmailSignupShowFamilyRegistration extends EmailSignupCustom with EquatableMixin {
const EmailSignupShowFamilyRegistration(this.email) {
+ assert(
+ email.isNotEmpty && email.contains('@'),
+ 'Email must be non-empty and contain @',
+ );
}
final String email;
+
+ @override
+ List<Object?> get props => [email];
}
-class EmailSignupShowFamilyLogin extends EmailSignupCustom {
+class EmailSignupShowFamilyLogin extends EmailSignupCustom with EquatableMixin {
const EmailSignupShowFamilyLogin(this.email) {
+ assert(
+ email.isNotEmpty && email.contains('@'),
+ 'Email must be non-empty and contain @',
+ );
}
final String email;
+
+ @override
+ List<Object?> get props => [email];
}📝 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.
| class EmailSignupShowFamilyRegistration extends EmailSignupCustom { | |
| const EmailSignupShowFamilyRegistration(this.email); | |
| final String email; | |
| } | |
| class EmailSignupShowFamilyLogin extends EmailSignupCustom { | |
| const EmailSignupShowFamilyLogin(this.email); | |
| final String email; | |
| } | |
| import 'package:equatable/equatable.dart'; | |
| class EmailSignupShowFamilyRegistration extends EmailSignupCustom with EquatableMixin { | |
| const EmailSignupShowFamilyRegistration(this.email) { | |
| assert( | |
| email.isNotEmpty && email.contains('@'), | |
| 'Email must be non-empty and contain @', | |
| ); | |
| } | |
| final String email; | |
| @override | |
| List<Object?> get props => [email]; | |
| } | |
| class EmailSignupShowFamilyLogin extends EmailSignupCustom with EquatableMixin { | |
| const EmailSignupShowFamilyLogin(this.email) { | |
| assert( | |
| email.isNotEmpty && email.contains('@'), | |
| 'Email must be non-empty and contain @', | |
| ); | |
| } | |
| final String email; | |
| @override | |
| List<Object?> get props => [email]; | |
| } |
| /// This should be moved to the CUBIT | ||
| Future<void> _checkAuthentication() async { | ||
| // Without biometrics we use the regular route to login | ||
| if (!await LocalAuthInfo.instance.canCheckBiometrics) return; | ||
|
|
||
| // When not authenticated do nothing | ||
| final hasAuthenticated = await LocalAuthInfo.instance.authenticate(); | ||
| if (!hasAuthenticated) return; | ||
|
|
||
| // When authenticated we go to the home route | ||
| if (!mounted) return; | ||
| await context.read<AuthCubit>().authenticate(); | ||
|
|
||
| if (!mounted) return; | ||
| context.goNamed(Pages.home.name); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor _checkAuthentication() Logic into the Cubit
The _checkAuthentication() method contains authentication logic that aligns with business rules and should reside within the EmailSignupCubit. Moving this logic into the cubit adheres to the BLoC pattern, promoting a clear separation between the presentation and business logic layers.
Consider refactoring as follows:
- Move the
_checkAuthentication()method intoEmailSignupCubitascheckAuthentication(). - Pass any required context or dependencies to the cubit method if necessary.
- Update
initState()to call_cubit.checkAuthentication().
Apply this diff to implement the changes:
class _EmailSignupPageState extends State<EmailSignupPage> {
// ...
@override
void initState() {
super.initState();
- _checkAuthentication();
+ _cubit.checkAuthentication(context);
}
- /// This should be moved to the CUBIT
- Future<void> _checkAuthentication() async {
- // Authentication logic here
- }
}
+// In email_signup_cubit.dart
+Future<void> checkAuthentication(BuildContext context) async {
+ // Move the logic from _checkAuthentication() here
+ // Ensure to manage any dependencies appropriately
+}Committable suggestion skipped: line range outside the PR's diff.
| SingleChildScrollView _buildLayout( | ||
| Object? state, BuildContext context, AppLocalizations locals) { | ||
| final user = (state! as Authenticated).user; | ||
| return SingleChildScrollView( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid force casting with ! and ensure state is Authenticated
The force cast (state! as Authenticated) assumes that state is non-null and of type Authenticated. While buildWhen filters for Authenticated states, it's safer to perform an explicit check.
Refactor the code as follows:
if (state is Authenticated) {
final user = state.user;
// Proceed with building the layout
} else {
// Handle other states or return a fallback widget
}This ensures that you handle any unexpected states gracefully and prevent potential runtime exceptions.
| child: BlocBuilder( | ||
| bloc: _authCubit, | ||
| buildWhen: (previous, current) => current is Authenticated, | ||
| builder: (context, state) { | ||
| return _buildLayout(state, context, locals); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Type safety in BlocBuilder and potential state mismatch
In the BlocBuilder, the buildWhen condition filters for states of type Authenticated, but the builder function accepts state as dynamic, which may lead to type issues if other states are emitted.
Apply this diff to enforce type safety:
-child: BlocBuilder(
- bloc: _authCubit,
- buildWhen: (previous, current) => current is Authenticated,
- builder: (context, state) {
+child: BlocBuilder<FamilyAuthCubit, FamilyAuthState>(
+ bloc: _authCubit,
+ buildWhen: (previous, current) => current is Authenticated,
+ builder: (context, FamilyAuthState state) {
return _buildLayout(state, context, locals);
},
),Additionally, consider handling other possible states or ensuring that only Authenticated state reaches _buildLayout to avoid runtime errors.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -90,227 +97,236 @@ class USPersonalInfoEditPage extends StatelessWidget { | |||
| /// if change is success refresh user that used in the cubit | |||
| if (state.status == PersonalInfoEditStatus.success) { | |||
| context | |||
| .read<AuthCubit>() | |||
| .read<FamilyAuthCubit>() | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors when refreshing the user
When calling refreshUser(), any exceptions thrown will be unhandled, which may lead to unexpected behavior or crashes.
Apply this diff to handle potential errors:
- .whenComplete(() => context.pop());
+ .then((_) => context.pop())
+ .catchError((error) {
+ // Handle the error, e.g., show an error message
+ showDialog<void>(
+ context: context,
+ builder: (_) => WarningDialog(
+ title: locals.errorTitle,
+ content: locals.refreshUserError,
+ onConfirm: () => context.pop(),
+ ),
+ );
+ });Ensure that you provide user feedback in case the refresh operation fails.
Committable suggestion skipped: line range outside the PR's diff.
| checkAuthRequest: CheckAuthRequest( | ||
| navigate: (context, {isUSUser}) => | ||
| showModalBottomSheet<void>( | ||
| context: context, | ||
| isScrollControlled: true, | ||
| useSafeArea: true, | ||
| builder: (_) => FingerprintBottomSheet( | ||
| isFingerprint: isFingerprintAvailable, | ||
| ), | ||
| ), | ||
| return Column( | ||
| children: [ | ||
| if (shouldShow) | ||
| _buildInfoRow( | ||
| context, | ||
| value: isFingerprintAvailable | ||
| ? Platform.isAndroid | ||
| ? locals.fingerprintTitle | ||
| : locals.touchId | ||
| : locals.faceId, | ||
| icon: Platform.isIOS && isFaceIdAvailable | ||
| ? SvgPicture.asset( | ||
| 'assets/images/face_id_image.svg', | ||
| width: 24, | ||
| ) | ||
| : const Icon(Icons.fingerprint), | ||
| onTap: () async => FamilyAuthUtils.authenticateUser( | ||
| context, | ||
| checkAuthRequest: FamilyCheckAuthRequest( | ||
| navigate: (context) => showModalBottomSheet<void>( | ||
| context: context, | ||
| isScrollControlled: true, | ||
| useSafeArea: true, | ||
| builder: (_) => FingerprintBottomSheet( | ||
| isFingerprint: isFingerprintAvailable, | ||
| ), | ||
| ), | ||
| ), | ||
| if (shouldShow) | ||
| const Divider( | ||
| height: 0, | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ), | ||
| const Divider( | ||
| height: 0, | ||
| ), | ||
| _buildInfoRow( | ||
| context, | ||
| icon: const Icon( | ||
| FontAwesomeIcons.userXmark, | ||
| ), | ||
| value: locals.unregister, | ||
| onTap: () async => AuthUtils.checkToken( | ||
| context, | ||
| checkAuthRequest: CheckAuthRequest( | ||
| navigate: (context, {isUSUser}) async => context.pushNamed( | ||
| FamilyPages.unregisterUS.name, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| const Divider( | ||
| height: 0, | ||
| ), | ||
| _buildInfoRow( | ||
| context, | ||
| icon: const Icon( | ||
| FontAwesomeIcons.circleInfo, | ||
| ), | ||
| value: locals.titleAboutGivt, | ||
| onTap: () => showModalBottomSheet<void>( | ||
| context: context, | ||
| isScrollControlled: true, | ||
| useSafeArea: true, | ||
| builder: (_) => Theme( | ||
| data: const FamilyAppTheme().toThemeData(), | ||
| child: const AboutGivtBottomSheet(), | ||
| ), | ||
| ), | ||
| ), | ||
| const Divider( | ||
| height: 0, | ||
| ), | ||
| _buildInfoRow( | ||
| context, | ||
| style: Theme.of(context).textTheme.labelMedium, | ||
| icon: const Icon( | ||
| FontAwesomeIcons.rightFromBracket, | ||
| if (shouldShow) | ||
| const Divider( | ||
| height: 0, | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify FutureBuilder logic and handle null safety
The FutureBuilder checks for snapshot.hasData, snapshot.data == null, and snapshot.connectionState, which can be simplified. Also, casting snapshot.data! as List<bool> without checking may lead to exceptions.
Apply this diff to simplify and enhance null safety:
builder: (_, snapshot) {
- if (snapshot.connectionState != ConnectionState.done) {
+ if (!snapshot.hasData || snapshot.connectionState != ConnectionState.done) {
return const SizedBox.shrink();
}
- if (!snapshot.hasData) {
- return const SizedBox.shrink();
- }
- if (snapshot.data == null) {
- return const SizedBox.shrink();
- }
- final data = snapshot.data! as List<bool>;
+ final data = snapshot.data as List<bool>;
final isFingerprintAvailable = data[0];
final isFaceIdAvailable = data[1];This reduces redundant checks and safely accesses snapshot.data.
📝 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.
| FutureBuilder( | |
| initialData: false, | |
| future: Future.wait<bool>([ | |
| LocalAuthInfo.instance.checkFingerprint(), | |
| LocalAuthInfo.instance.checkFaceId(), | |
| ]), | |
| builder: (_, snapshot) { | |
| if (snapshot.connectionState != ConnectionState.done) { | |
| return const SizedBox.shrink(); | |
| } | |
| if (!snapshot.hasData) { | |
| return const SizedBox.shrink(); | |
| } | |
| if (snapshot.data == null) { | |
| return const SizedBox.shrink(); | |
| } | |
| final data = snapshot.data! as List<bool>; | |
| final isFingerprintAvailable = data[0]; | |
| final isFaceIdAvailable = data[1]; | |
| final shouldShow = | |
| isFingerprintAvailable || isFaceIdAvailable; | |
| final data = snapshot.data! as List<bool>; | |
| final isFingerprintAvailable = data[0]; | |
| final isFaceIdAvailable = data[1]; | |
| final shouldShow = isFingerprintAvailable || isFaceIdAvailable; | |
| return Column( | |
| children: [ | |
| if (shouldShow) | |
| _buildInfoRow( | |
| context, | |
| value: isFingerprintAvailable | |
| ? Platform.isAndroid | |
| ? locals.fingerprintTitle | |
| : locals.touchId | |
| : locals.faceId, | |
| icon: Platform.isIOS && isFaceIdAvailable | |
| ? SvgPicture.asset( | |
| 'assets/images/face_id_image.svg', | |
| width: 24, | |
| ) | |
| : const Icon(Icons.fingerprint), | |
| onTap: () async => AuthUtils.checkToken( | |
| context, | |
| checkAuthRequest: CheckAuthRequest( | |
| navigate: (context, {isUSUser}) => | |
| showModalBottomSheet<void>( | |
| context: context, | |
| isScrollControlled: true, | |
| useSafeArea: true, | |
| builder: (_) => FingerprintBottomSheet( | |
| isFingerprint: isFingerprintAvailable, | |
| ), | |
| ), | |
| return Column( | |
| children: [ | |
| if (shouldShow) | |
| _buildInfoRow( | |
| context, | |
| value: isFingerprintAvailable | |
| ? Platform.isAndroid | |
| ? locals.fingerprintTitle | |
| : locals.touchId | |
| : locals.faceId, | |
| icon: Platform.isIOS && isFaceIdAvailable | |
| ? SvgPicture.asset( | |
| 'assets/images/face_id_image.svg', | |
| width: 24, | |
| ) | |
| : const Icon(Icons.fingerprint), | |
| onTap: () async => FamilyAuthUtils.authenticateUser( | |
| context, | |
| checkAuthRequest: FamilyCheckAuthRequest( | |
| navigate: (context) => showModalBottomSheet<void>( | |
| context: context, | |
| isScrollControlled: true, | |
| useSafeArea: true, | |
| builder: (_) => FingerprintBottomSheet( | |
| isFingerprint: isFingerprintAvailable, | |
| ), | |
| ), | |
| ), | |
| if (shouldShow) | |
| const Divider( | |
| height: 0, | |
| ), | |
| ], | |
| ); | |
| }, | |
| ), | |
| const Divider( | |
| height: 0, | |
| ), | |
| _buildInfoRow( | |
| context, | |
| icon: const Icon( | |
| FontAwesomeIcons.userXmark, | |
| ), | |
| value: locals.unregister, | |
| onTap: () async => AuthUtils.checkToken( | |
| context, | |
| checkAuthRequest: CheckAuthRequest( | |
| navigate: (context, {isUSUser}) async => context.pushNamed( | |
| FamilyPages.unregisterUS.name, | |
| ), | |
| ), | |
| ), | |
| ), | |
| ), | |
| const Divider( | |
| height: 0, | |
| ), | |
| _buildInfoRow( | |
| context, | |
| icon: const Icon( | |
| FontAwesomeIcons.circleInfo, | |
| ), | |
| value: locals.titleAboutGivt, | |
| onTap: () => showModalBottomSheet<void>( | |
| context: context, | |
| isScrollControlled: true, | |
| useSafeArea: true, | |
| builder: (_) => Theme( | |
| data: const FamilyAppTheme().toThemeData(), | |
| child: const AboutGivtBottomSheet(), | |
| ), | |
| ), | |
| ), | |
| const Divider( | |
| height: 0, | |
| ), | |
| _buildInfoRow( | |
| context, | |
| style: Theme.of(context).textTheme.labelMedium, | |
| icon: const Icon( | |
| FontAwesomeIcons.rightFromBracket, | |
| if (shouldShow) | |
| const Divider( | |
| height: 0, | |
| ), | |
| ], | |
| ); | |
| }, | |
| ), | |
| FutureBuilder( | |
| initialData: false, | |
| future: Future.wait<bool>([ | |
| LocalAuthInfo.instance.checkFingerprint(), | |
| LocalAuthInfo.instance.checkFaceId(), | |
| ]), | |
| builder: (_, snapshot) { | |
| if (!snapshot.hasData || snapshot.connectionState != ConnectionState.done) { | |
| return const SizedBox.shrink(); | |
| } | |
| final data = snapshot.data as List<bool>; | |
| final isFingerprintAvailable = data[0]; | |
| final isFaceIdAvailable = data[1]; | |
| final shouldShow = isFingerprintAvailable || isFaceIdAvailable; | |
| return Column( | |
| children: [ | |
| if (shouldShow) | |
| _buildInfoRow( | |
| context, | |
| value: isFingerprintAvailable | |
| ? Platform.isAndroid | |
| ? locals.fingerprintTitle | |
| : locals.touchId | |
| : locals.faceId, | |
| icon: Platform.isIOS && isFaceIdAvailable | |
| ? SvgPicture.asset( | |
| 'assets/images/face_id_image.svg', | |
| width: 24, | |
| ) | |
| : const Icon(Icons.fingerprint), | |
| onTap: () async => FamilyAuthUtils.authenticateUser( | |
| context, | |
| checkAuthRequest: FamilyCheckAuthRequest( | |
| navigate: (context) => showModalBottomSheet<void>( | |
| context: context, | |
| isScrollControlled: true, | |
| useSafeArea: true, | |
| builder: (_) => FingerprintBottomSheet( | |
| isFingerprint: isFingerprintAvailable, | |
| ), | |
| ), | |
| ), | |
| ), | |
| ), | |
| if (shouldShow) | |
| const Divider( | |
| height: 0, | |
| ), | |
| ], | |
| ); | |
| }, | |
| ), |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
lib/features/family/features/registration/cubit/us_signup_custom.dart (1)
10-16: Navigation states look good but consider documentationThe navigation state implementations are clean and follow the pattern correctly.
Consider adding documentation to clarify when each state should be used:
+/// State indicating user should be redirected to add family members screen class UsSignupRedirectToAddMembers extends UsSignupCustom { const UsSignupRedirectToAddMembers(); } +/// State indicating user should be redirected to home screen class UsSignupRedirectToHome extends UsSignupCustom { const UsSignupRedirectToHome(); }lib/features/impact_groups/widgets/impact_group_recieve_invite_sheet.dart (2)
25-53: Consider adding spacing between Column childrenThe Column layout could benefit from proper spacing between elements for better visual hierarchy.
body: Column( mainAxisAlignment: MainAxisAlignment.spaceBetween, children: [ + const SizedBox(height: 24), Text( '${context.l10n.youHaveBeenInvitedToImpactGroup} ${invitdImpactGroup.name}', textAlign: TextAlign.center, style: GoogleFonts.mulish( textStyle: Theme.of(context).textTheme.titleLarge?.copyWith( fontWeight: FontWeight.w700, ), ), ), + const SizedBox(height: 24), SvgPicture.asset('assets/images/family_superheroes.svg'), + const SizedBox(height: 24), FunButton( // ... button implementation ), + const SizedBox(height: 16), ], ),
The verification results confirm both issues and their impact across multiple files. I'll generate the final response:
Fix typos in file name and variable name across multiple files
The typos affect multiple files in the codebase:
File name
impact_group_recieve_invite_sheet.dartis referenced in:
lib/features/give/pages/home_page.dartlib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dartVariable name
invitdImpactGroupis used in:
lib/features/give/pages/home_page.dartlib/features/impact_groups/widgets/impact_group_recieve_invite_sheet.dartlib/features/family/features/home_screen/presentation/pages/navigation_bar_home_screen.dartAll these occurrences need to be updated to use the correct spelling:
- Rename file to
impact_group_receive_invite_sheet.dart- Rename variable to
invitedImpactGroup🔗 Analysis chain
Line range hint
1-1: Fix typos in file name and variable name
The file name contains a typo: "recieve" should be "receive"
The parameter name contains a typo: "invitdImpactGroup" should be "invitedImpactGroup"
Rename the file to:
lib/features/impact_groups/widgets/impact_group_receive_invite_sheet.dart
- Fix the variable name:
class ImpactGroupReceiveInviteSheet extends StatelessWidget { const ImpactGroupReceiveInviteSheet({ - required this.invitdImpactGroup, + required this.invitedImpactGroup, super.key, }); - final ImpactGroup invitdImpactGroup; + final ImpactGroup invitedImpactGroup;Remember to update all references to this variable throughout the file.
Also applies to: 15-18
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all references to the misspelled file name rg -l "impact_group_recieve_invite" # Find all references to the misspelled variable rg "invitdImpactGroup"Length of output: 1091
lib/features/family/app/injection.dart (2)
124-134: Document dependencies for better maintainability.While the registration of cubits follows good practices (singletons for auth state, factories for screen-specific cubits), consider adding documentation to clarify the required dependencies for each cubit. This will make it easier to maintain and understand the dependency graph.
Example:
// FamilyAuthCubit(FamilyAuthRepository) ..registerLazySingleton<FamilyAuthCubit>( () => FamilyAuthCubit(getIt()), ) // UsSignupCubit(FamilyAuthRepository, ProfilesRepository) ..registerFactory<UsSignupCubit>( () => UsSignupCubit(getIt(), getIt()), )
145-150: Document dependencies for FamilyAuthRepository.Similar to the cubits, consider documenting the dependencies being injected into FamilyAuthRepositoryImpl for better maintainability.
Example:
// FamilyAuthRepositoryImpl(FamilyAPIService, RequestHelper) ..registerLazySingleton<FamilyAuthRepository>( () => FamilyAuthRepositoryImpl( getIt(), getIt(), ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
lib/features/family/app/injection.dart(6 hunks)lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart(2 hunks)lib/features/family/features/registration/cubit/us_signup_cubit.dart(1 hunks)lib/features/family/features/registration/cubit/us_signup_custom.dart(1 hunks)lib/features/family/features/registration/pages/us_signup_page.dart(4 hunks)lib/features/impact_groups/widgets/impact_group_recieve_invite_sheet.dart(2 hunks)lib/features/splash/cubit/splash_cubit.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/family/features/home_screen/cubit/navigation_bar_home_cubit.dart
- lib/features/splash/cubit/splash_cubit.dart
🔇 Additional comments (12)
lib/features/family/features/registration/cubit/us_signup_custom.dart (3)
1-8: Good use of sealed class for state management
The sealed class pattern is well-suited for managing finite state variations in the signup flow. The use of const constructors and factory methods promotes immutability and clean instantiation.
1-16:
Additional states needed for complete signup flow
The current implementation only handles navigation states. As noted in the previous review, a complete signup flow typically needs additional states.
Building upon the previous review comment, here's an expanded suggestion that includes loading indicators and error handling while preserving the new navigation states:
sealed class UsSignupCustom {
const UsSignupCustom();
+ const factory UsSignupCustom.initial() = UsSignupCustomInitial;
+ const factory UsSignupCustom.loading() = UsSignupCustomLoading;
+ const factory UsSignupCustom.error(String message) = UsSignupCustomError;
const factory UsSignupCustom.redirectToAddMembers() = UsSignupRedirectToAddMembers;
const factory UsSignupCustom.redirectToHome() = UsSignupRedirectToHome;
}
+class UsSignupCustomInitial extends UsSignupCustom {
+ const UsSignupCustomInitial();
+}
+
+class UsSignupCustomLoading extends UsSignupCustom {
+ const UsSignupCustomLoading();
+}
+
+class UsSignupCustomError extends UsSignupCustom {
+ final String message;
+ const UsSignupCustomError(this.message);
+}
class UsSignupRedirectToAddMembers extends UsSignupCustom {
const UsSignupRedirectToAddMembers();
}
class UsSignupRedirectToHome extends UsSignupCustom {
const UsSignupRedirectToHome();
}4-7: Verify navigation state transitions
Since the PR aims to address "double screen bugs and forced navigation", ensure that transitions between these navigation states are properly managed in the corresponding cubit.
✅ Verification successful
Navigation state transitions are properly implemented
The navigation states are correctly managed in the UsSignupCubit:
- Navigation to home or add members is handled based on whether the user is invited to a group
- State transitions are properly emitted using
emitCustom - The page correctly handles these navigation states in the
handleCustommethod - Error cases are properly handled with snackbar messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the cubit implementation that manages these states
ast-grep --pattern 'class $_ extends Cubit<UsSignupCustom> {
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search for the cubit implementation
rg -l "UsSignupCustom" --type dart
# Then search for any state transitions or emit calls related to these navigation states
rg "emit\(.*(?:redirectToAddMembers|redirectToHome)" --type dart
Length of output: 308
Script:
#!/bin/bash
# Let's examine the content of these files to understand the navigation state handling
echo "=== Content of us_signup_cubit.dart ==="
cat lib/features/family/features/registration/cubit/us_signup_cubit.dart
echo -e "\n=== Content of us_signup_page.dart ==="
cat lib/features/family/features/registration/pages/us_signup_page.dart
Length of output: 14656
lib/features/impact_groups/widgets/impact_group_recieve_invite_sheet.dart (1)
10-10: LGTM: Import statement is correctly placed
The import for the new FunScaffold widget follows Flutter conventions.
lib/features/family/features/registration/cubit/us_signup_cubit.dart (2)
1-20: LGTM! Well-structured class with proper dependency injection.
The class follows good practices with clear separation of concerns and proper dependency injection through the constructor.
71-74: Clarify the purpose of hardcoded isNewUser flag.
The isNewUser parameter is hardcoded to false without explanation. This could lead to confusion and potential bugs.
Let's check how this flag is used:
✅ Verification successful
The isNewUser flag usage is consistent and purposeful
The isNewUser: false flag in the US signup flow is correct. Looking at the codebase:
- Initial email signup uses
isNewUser: true(in email_signup_cubit.dart) - Family member and registration flows use
isNewUser: false(in us_signup_cubit.dart and registration_bloc.dart)
This pattern indicates the flag is used to differentiate between brand new users signing up for the first time versus additional family members or completing registration of existing users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for registerUser method implementation and usages
ast-grep --pattern 'registerUser($$$)'
Length of output: 40
Script:
#!/bin/bash
# Search for registerUser method and its usages with ripgrep
rg "registerUser" -A 5 -B 5
Length of output: 12298
lib/features/family/app/injection.dart (1)
145-150: Verify EU/US logic handling in FamilyAuthRepository.
As per PR objectives, this refactor aims to address the complexities of different logic requirements between EU and US. Let's verify the implementation.
lib/features/family/features/registration/pages/us_signup_page.dart (5)
7-31: LGTM! Clean transition to Cubit architecture
The import changes and initialization of the Cubit via dependency injection align well with the refactoring objectives. The explicit Country.us initialization improves code clarity.
Also applies to: 56-59
60-82: LGTM! Proper lifecycle management
The lifecycle methods are properly implemented with appropriate Cubit initialization and cleanup.
86-94: LGTM! Clean state management implementation
The BaseStateConsumer provides a cleaner approach to state management, with proper handling of loading and custom states for navigation.
Also applies to: 152-159
Line range hint 223-341: LGTM! Robust form validation implementation
The form validation logic is thorough and properly implements all required checks for:
- Name fields with regex validation
- Phone number format based on country
- Password complexity requirements
Line range hint 1-341: Verify separation of EU/US specific logic
As part of the auth refactor to address EU/US differences, let's verify that US-specific logic is properly isolated.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient evidence about the separation of EU/US logic.
US-specific logic is properly isolated in dedicated files
The codebase shows clear separation of US-specific logic through dedicated files:
- Separate US signup implementation in
features/family/features/registration/with dedicated files:us_signup_page.dartus_signup_cubit.dartus_mobile_number_form_field.dartaccept_policy_row_us.dart
- General registration logic remains in
features/registration/without country-specific implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining mixed EU/US logic in auth-related files
# Look for EU/US conditional checks in auth files
rg -l "(?i)(EU|US|europe|united states)" --type dart "lib/features/**/auth/"
# Look for country-specific logic in auth files
ast-grep --pattern 'if ($_ == Country.us || $_ == Country.eu) { $$$ }'
Length of output: 213
Script:
#!/bin/bash
# Let's try a broader search to find auth-related files and check for EU/US specific logic
# Find all registration/signup related files
fd -e dart "(registration|signup|auth)" lib/features
# Search for country-specific logic in these files
rg -l "Country\.(us|eu)" lib/features
# Look for any EU/US specific class/file names
fd -e dart "(eu|us).*\.(dart)" lib/features
Length of output: 3697
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: [ | ||
| Text( | ||
| '${context.l10n.youHaveBeenInvitedToImpactGroup}${invitdImpactGroup.name}', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper spacing in the text concatenation
The concatenated string lacks proper spacing between the message and the group name.
- '${context.l10n.youHaveBeenInvitedToImpactGroup}${invitdImpactGroup.name}',
+ '${context.l10n.youHaveBeenInvitedToImpactGroup} ${invitdImpactGroup.name}',📝 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.
| '${context.l10n.youHaveBeenInvitedToImpactGroup}${invitdImpactGroup.name}', | |
| '${context.l10n.youHaveBeenInvitedToImpactGroup} ${invitdImpactGroup.name}', |
| Future<void> init() async { | ||
| try { | ||
| await _authRepository.refreshToken(); | ||
| } catch (_) { | ||
| // do nothing, failing is fine | ||
| } | ||
| final user = _authRepository.getCurrentUser(); | ||
|
|
||
| if (user == null) { | ||
| emitError(null); | ||
| return; | ||
| } | ||
|
|
||
| emitData(user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging in init() method.
Several concerns with the current implementation:
- The empty catch block needs better documentation explaining why failing is acceptable.
- Emitting null error provides no context for debugging or user feedback.
- Consider logging failed refresh attempts for debugging purposes.
Consider this improvement:
Future<void> init() async {
try {
await _authRepository.refreshToken();
} catch (e) {
- // do nothing, failing is fine
+ // Token refresh failures are expected for new users or expired sessions
+ // Logging for debugging purposes only
+ print('Token refresh failed: $e');
}
final user = _authRepository.getCurrentUser();
if (user == null) {
- emitError(null);
+ emitError('User not found or session expired');
return;
}
emitData(user);
}📝 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.
| Future<void> init() async { | |
| try { | |
| await _authRepository.refreshToken(); | |
| } catch (_) { | |
| // do nothing, failing is fine | |
| } | |
| final user = _authRepository.getCurrentUser(); | |
| if (user == null) { | |
| emitError(null); | |
| return; | |
| } | |
| emitData(user); | |
| } | |
| Future<void> init() async { | |
| try { | |
| await _authRepository.refreshToken(); | |
| } catch (e) { | |
| // Token refresh failures are expected for new users or expired sessions | |
| // Logging for debugging purposes only | |
| print('Token refresh failed: $e'); | |
| } | |
| final user = _authRepository.getCurrentUser(); | |
| if (user == null) { | |
| emitError('User not found or session expired'); | |
| return; | |
| } | |
| emitData(user); | |
| } |
| amountLimit: | ||
| country.toUpperCase() == Country.us.countryCode ? 4999 : 499, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract amount limits to constants or configuration.
The hardcoded amount limits (4999 for US, 499 for others) should be moved to a configuration file or constants class for better maintainability.
+ // In a constants file
+ class AmountLimits {
+ static const usLimit = 4999;
+ static const defaultLimit = 499;
+ }
// In the cubit
amountLimit:
- country.toUpperCase() == Country.us.countryCode ? 4999 : 499,
+ country.toUpperCase() == Country.us.countryCode
+ ? AmountLimits.usLimit
+ : AmountLimits.defaultLimit,Committable suggestion skipped: line range outside the PR's diff.
| Future<void> savePersonalInfo({ | ||
| required String email, | ||
| required String password, | ||
| required String firstName, | ||
| required String lastName, | ||
| required String country, | ||
| required String phoneNumber, | ||
| required String appLanguage, | ||
| required String countryCode, | ||
| required String profilePicture, | ||
| }) async { | ||
| emitLoading(); | ||
|
|
||
| try { | ||
| final tempUser = TempUser( | ||
| email: email, | ||
| country: country, | ||
| appLanguage: appLanguage, | ||
| timeZoneId: await FlutterTimezone.getLocalTimezone(), | ||
| amountLimit: | ||
| country.toUpperCase() == Country.us.countryCode ? 4999 : 499, | ||
| address: Util.defaultAddress, | ||
| city: Util.defaultCity, | ||
| postalCode: Util.defaultPostCode, | ||
| firstName: firstName, | ||
| iban: Util.defaultIban, | ||
| lastName: lastName, | ||
| password: password, | ||
| phoneNumber: phoneNumber, | ||
| accountNumber: Util.empty, | ||
| sortCode: Util.empty, | ||
| profilePicture: profilePicture, | ||
| ); | ||
|
|
||
| await _authRepository.registerUser( | ||
| tempUser: tempUser, | ||
| isNewUser: false, | ||
| ); | ||
|
|
||
| final invitedGroup = await _impactGroupsRepository.isInvitedToGroup(); | ||
|
|
||
| if (invitedGroup != null) { | ||
| emitCustom(const UsSignupRedirectToHome()); | ||
| return; | ||
| } | ||
|
|
||
| emitCustom(const UsSignupRedirectToAddMembers()); | ||
| } catch (e) { | ||
| emitSnackbarMessage(e.toString(), isError: true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation and improve error handling.
The method needs several improvements:
- Input validation for email, password, phone number formats
- Secure password handling
- More specific error handling instead of using toString()
Consider implementing these improvements:
+ // Add validation methods
+ bool _isValidEmail(String email) {
+ return RegExp(r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$').hasMatch(email);
+ }
+
+ bool _isValidPassword(String password) {
+ // Add your password strength requirements
+ return password.length >= 8;
+ }
+
+ bool _isValidPhoneNumber(String phoneNumber, String countryCode) {
+ // Add phone number validation logic
+ return phoneNumber.isNotEmpty;
+ }
Future<void> savePersonalInfo({
required String email,
required String password,
// ... other parameters
}) async {
emitLoading();
try {
+ // Validate inputs
+ if (!_isValidEmail(email)) {
+ throw ValidationException('Invalid email format');
+ }
+ if (!_isValidPassword(password)) {
+ throw ValidationException('Password must be at least 8 characters');
+ }
+ if (!_isValidPhoneNumber(phoneNumber, countryCode)) {
+ throw ValidationException('Invalid phone number');
+ }
final tempUser = TempUser(
// ... existing code
);
await _authRepository.registerUser(
tempUser: tempUser,
isNewUser: false,
);
final invitedGroup = await _impactGroupsRepository.isInvitedToGroup();
if (invitedGroup != null) {
emitCustom(const UsSignupRedirectToHome());
return;
}
emitCustom(const UsSignupRedirectToAddMembers());
} catch (e) {
- emitSnackbarMessage(e.toString(), isError: true);
+ if (e is ValidationException) {
+ emitSnackbarMessage(e.message, isError: true);
+ } else {
+ emitSnackbarMessage('Registration failed. Please try again.', isError: true);
+ // Log the actual error for debugging
+ print('Registration error: $e');
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
lib/features/family/features/bedtime/presentation/pages/setup_bedtime_screen.dart (2)
84-84: Address the TODO comment for city animation.While the background color change to use
FamilyAppTheme.secondary10improves consistency, there's a TODO comment about animating the city before page navigation that should be addressed.Would you like me to help implement the city animation or create a GitHub issue to track this task?
Also applies to: 86-86
Line range hint
67-67: Address the TODO comment for mission screen navigation.There's a pending TODO about navigation to the mission screen that needs to be implemented.
Would you like me to help implement the navigation logic or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/features/family/features/bedtime/presentation/pages/setup_bedtime_screen.dart(3 hunks)lib/features/family/utils/family_app_theme.dart(1 hunks)
🔇 Additional comments (3)
lib/features/family/features/bedtime/presentation/pages/setup_bedtime_screen.dart (3)
14-14: LGTM: New imports are properly organized and utilized.
The added imports for UI components align with the changes and follow Flutter's import organization conventions.
Also applies to: 16-16, 21-21
59-62: LGTM: Loading state UI improvements enhance consistency.
The loading state now uses shared components (FunScaffold, CustomCircularProgressIndicator) and maintains visual consistency with the main screen's background color.
Line range hint 193-201: Review PII handling in analytics event.
The analytics event includes the child's first name. Consider whether this level of PII is necessary for analytics purposes or if it should be anonymized.
Let's check other analytics events in the codebase for consistency in PII handling:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
lib/features/family/features/login/presentation/pages/family_login_sheet.dart (3)
113-117: Remove unnecessaryonChangedcallback for read-only email field.The email field is marked as
readOnly, so theonChangedcallback will not be triggered. This code can be removed to clean up the form field definition.Apply this diff to remove the unnecessary
onChanged:OutlinedTextFormField( controller: emailController, keyboardType: TextInputType.emailAddress, readOnly: true, autofillHints: const [ AutofillHints.username, AutofillHints.email, ], - onChanged: (value) { - setState(() { - formKey.currentState!.validate(); - }); - }, validator: (value) { if (value == null || value.isEmpty || !Util.emailRegEx.hasMatch(value)) { return context.l10n.invalidEmail; } return null; }, hintText: context.l10n.email, ),
65-67: SimplifyisEnabledgetter logic.The
isEnabledgetter redundantly checks the validation state and the emptiness of the text fields. Since the validators already ensure that the fields are not empty, you can simplify the logic by relying solely on the validation result.Consider this simplified version:
bool get isEnabled { - if (formKey.currentState == null) return false; - if (formKey.currentState!.validate() == false) return false; - return emailController.text.isNotEmpty && - passwordController.text.isNotEmpty; + return formKey.currentState?.validate() ?? false; }
134-137: Optimize form validation call insideonChanged.Calling
formKey.currentState!.validate()insidesetStatemay lead to unnecessary rebuilds. Consider validating only the specific field or debouncing the validation calls.Refactor the
onChangedmethod:onChanged: (value) { - setState(() { - formKey.currentState!.validate(); - }); + formKey.currentState!.validate(); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/features/family/features/login/presentation/pages/family_login_sheet.dart(1 hunks)
🔇 Additional comments (3)
lib/features/family/features/login/presentation/pages/family_login_sheet.dart (3)
47-52: Dispose TextEditingControllers to prevent memory leaks.
The emailController and passwordController should be disposed of in the dispose method to free up resources when the widget is removed from the tree.
138-153: Refactor password validation logic for maintainability and security.
Consider extracting the password validation logic into a separate method to improve readability and maintainability. This also helps in reusing the validation logic if needed elsewhere.
195-243: Reduce code duplication in dialog handling.
The dialog creation code within the onCustom method is duplicated across different cases with minimal differences. Refactoring this into a separate method will enhance code clarity and reduce redundancy.
|
|
||
| Widget showLoginForm(String email) { | ||
| emailController.text = email; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid directly setting emailController.text inside the build method.
Assigning values to controllers inside the build method can lead to unintentional side effects and performance issues. Consider moving this assignment to the initState or didChangeDependencies method.
Apply this diff to move the assignment:
Widget showLoginForm(String email) {
- emailController.text = email;
+ // Remove the assignment here
}
+@override
+void didChangeDependencies() {
+ super.didChangeDependencies();
+ _cubit.init(widget.email);
+ emailController.text = widget.email ?? '';
+}Committable suggestion skipped: line range outside the PR's diff.
| cubit: _cubit, | ||
| onCustom: onCustom, | ||
| onData: (context, data) => showLoginForm(data), | ||
| onLoading: (context) { | ||
| return FunBottomSheet( | ||
| title: context.l10n.login, | ||
| icon: const CustomCircularProgressIndicator(), | ||
| content: const BodyMediumText( | ||
| "We're logging you in", | ||
| ), | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Handle potential null value in onData callback.
In the BaseStateConsumer, the onData method receives a String email. Ensure that email is not null before using it, or handle the null case appropriately to prevent runtime exceptions.
Modify the onData callback to handle null email:
onData: (context, data) => showLoginForm(data),
+onError: (context, error) {
+ // Handle error appropriately
+},Alternatively, update the showLoginForm method to handle null values for email.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
EmailSignupCubitfor managing email signup state.FamilyAuthCubitfor family authentication management.SplashCubitfor splash screen navigation and state management.UsSignupCubitfor handling user sign-up processes.AvatarSelectionBottomsheetfor avatar selection functionality.MobileNumberFormFieldfor mobile number input with country prefix selection.RegisteredCheckAnimationfor visual feedback during registration.FamilyLoginCubitfor managing family login processes.FamilyLoginSheetfor streamlined user login functionality.CustomProgressIndicatorandFamilyAppThemefor enhanced UI components.Bug Fixes
TempUserandUtilclasses for default values.Refactor
isUSUser).ScaffoldwithFunScaffoldinImpactGroupReceiveInviteSheetfor improved UI structure.Documentation
Chores