Skip to content

Fix/better gps addres selector #36#42

Merged
abelarismendy merged 9 commits intomainfrom
fix/better-gps-addres-selector-#36
Oct 2, 2024
Merged

Fix/better gps addres selector #36#42
abelarismendy merged 9 commits intomainfrom
fix/better-gps-addres-selector-#36

Conversation

@abelarismendy
Copy link
Copy Markdown
Contributor

@abelarismendy abelarismendy commented Oct 2, 2024

closes #36

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new Address class for better address management, including properties for full address and location.
    • Enhanced HomeScreen with dynamic address display based on user interactions.
    • Added functionality for saving and removing user IDs in shared preferences during authentication.
  • Bug Fixes

    • Improved location handling and permissions in the AddressScreen.
  • Chores

    • Updated .gitignore to ignore .env files for security.
    • Added environment variable support by including the .env file as an asset.
  • Refactor

    • Restructured AddressBloc and event handling for improved state management.
    • Transitioned HomeScreen to a StatefulWidget for better state handling.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 2, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request includes modifications to various files, primarily focusing on configuration management and state handling. Key changes involve updating the .gitignore to exclude .env files, enhancing spell check settings in .vscode/settings.json, restructuring address management in the BLoC pattern, and introducing a new Address class. Additionally, the HomeScreen transitions to a stateful widget, and user ID management is improved in the AuthBloc by utilizing shared preferences.

Changes

File Path Change Summary
.gitignore Added entry for .env to ignore environment variable files.
.vscode/settings.json Updated cSpell.words to include "dotenv" and "Geolocator".
lib/core/ui/layouts/main_layout.dart Added imports for HomeBloc and flutter_bloc, wrapped HomeScreen in BlocProvider.
lib/core/utils/user_util.dart Introduced getUserId function to retrieve user ID from shared preferences.
lib/features/address/domain/address.dart Removed Address class that defined address structure and serialization methods.
lib/features/address/domain/models/address.dart Added new Address class with properties and methods for address management.
lib/features/address/presentation/bloc/address_bloc.dart Modified AddressBloc constructor, updated event handling methods, and introduced ClearAddress event.
lib/features/address/presentation/bloc/address_event.dart Simplified SaveAddress class and renamed LoadAddresses to LoadAddress, added ClearAddress class.
lib/features/address/presentation/bloc/address_state.dart Updated AddressLoaded to hold a single Address instead of a list, removed AddressSaved class.
lib/features/address/presentation/screens/address_detail_screen.dart Removed AddressDetailsScreen class.
lib/features/address/presentation/screens/address_screen.dart Enhanced functionality with new methods for location handling and improved UI structure.
lib/features/address/repository/address_repository.dart Updated import for Address model and reformatted fetchUserAddresses method.
lib/features/auth/presentation/bloc/auth_bloc.dart Added methods for saving and removing user ID in shared preferences, reformatted event handlers.
lib/features/home/presentation/screens/home_screen.dart Transitioned HomeScreen to a stateful widget, added state management for address data.
lib/main.dart Added AddressBloc to MultiBlocProvider, included loading environment variables from .env.
pubspec.yaml Added .env file to assets for environment variable access.

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • RaulRinconX

Poem

🐇 In the burrow, changes bloom,
.env files find their room.
Addresses now have a new face,
With BLoC magic, they find their place.
From home to auth, all flows align,
Hopping forward, all is fine! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@RaulRinconX RaulRinconX added enhancement New feature or request bug Something isn't working labels Oct 2, 2024
@RaulRinconX RaulRinconX added this to the Sprint 2 milestone Oct 2, 2024
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (22)
.vscode/settings.json (1)

5-7: LGTM! Consider adding more context.

The additions to the cSpell.words array are appropriate and align with the PR objectives. "Geolocator" is likely related to the GPS functionality, and "dotenv" suggests the use of environment variables for configuration management.

Consider adding a comment in the PR description or commit message explaining why these words were added. This can help future contributors understand the context of these additions.

lib/core/utils/user_util.dart (3)

3-6: Function implementation looks good, but consider adding error handling

The getUserId function is well-implemented and follows Dart's asynchronous programming practices. However, consider adding error handling to make the function more robust.

Here's a suggestion for adding error handling:

Future<String?> getUserId() async {
  try {
    final SharedPreferences prefs = await SharedPreferences.getInstance();
    return prefs.getString('userId');
  } catch (e) {
    print('Error retrieving user ID: $e');
    return null;
  }
}

This implementation will catch any exceptions that might occur when accessing SharedPreferences and return null in case of an error.


3-6: Add documentation comments

To improve code readability and maintainability, consider adding documentation comments to the getUserId function.

Here's a suggestion:

/// Retrieves the user ID from SharedPreferences.
///
/// Returns the stored user ID as a [String] if found, or `null` if not set.
/// This function is asynchronous and returns a [Future].
Future<String?> getUserId() async {
  // ... (existing implementation)
}

1-6: Consider future extensibility

The file name 'user_util.dart' suggests it might contain more user-related utility functions in the future. As the codebase grows, consider grouping related functions in this file and potentially creating a UserUtil class for better organization.

lib/features/address/presentation/bloc/address_event.dart (1)

1-12: Overall simplification of address events, verify system-wide impact.

The changes in this file simplify the address-related events, which is a good improvement. The removal of userId from events and the addition of a ClearAddress event suggest a shift in how addresses are handled in the system.

To ensure these changes are properly integrated:

  1. Update the AddressBloc to handle the new ClearAddress event.
  2. Modify any UI components that interact with these events.
  3. Update any tests related to address functionality.
  4. Review the address repository and use case layers to ensure they align with these event changes.
lib/features/address/presentation/bloc/address_state.dart (1)

Line range hint 1-18: Summary of changes and potential impact

The changes in this file seem to be moving towards a simpler, single-address model for the AddressState. This aligns with the PR objective of improving the GPS address selector (#36). The main changes are:

  1. Updated import from domain/address.dart to models/address.dart
  2. Modified AddressLoaded to hold a single Address instead of a list
  3. Removed the AddressSaved state

While these changes appear to simplify the address state management, they could have a significant impact on the overall functionality of the address selection process in the app. It's crucial to ensure that these changes are consistently applied throughout the codebase and that all affected components are updated accordingly.

Consider adding comments to explain the rationale behind these architectural changes, especially regarding the shift to a single-address model and the removal of the AddressSaved state. This would help maintain clarity for future development and code reviews.

.gitignore (1)

45-45: Consider adding a comment for clarity

While the purpose of ignoring .env files is generally understood among developers, it's always helpful to add a brief comment explaining why certain files are ignored, especially for team members who might be less familiar with the practice.

Consider adding a comment above the .env entry:

+# Ignore environment configuration files
.env
lib/features/address/domain/models/address.dart (1)

25-32: Consider using Dart's shorthand syntax for identical key-value pairs.

The toMap method correctly serializes all properties of the Address object. However, you can make it more concise by using Dart's shorthand syntax for identical key-value pairs. Here's a suggested improvement:

Map<String, dynamic> toMap() {
  return {
    'fullAddress': fullAddress,
    'latitude': latitude,
    'longitude': longitude,
    'deliveryDetails': deliveryDetails,
  };
}

This change maintains the same functionality while making the code slightly more readable and idiomatic Dart.

lib/core/ui/layouts/main_layout.dart (1)

22-25: LGTM: HomeScreen wrapped with BlocProvider.

The HomeScreen is now correctly wrapped with BlocProvider, introducing BLoC state management. This is a good practice for separating business logic from the UI.

However, consider the following optimization:

If MainLayout is rebuilt frequently, creating a new HomeBloc instance each time might impact performance. Consider moving the BlocProvider higher up in the widget tree or using BlocProvider.value if the HomeBloc can be shared across rebuilds.

Example:

class MainLayout extends StatefulWidget {
  final HomeBloc homeBloc;
  
  const MainLayout({super.key, required this.homeBloc});

  @override
  MainLayoutState createState() => MainLayoutState();
}

class MainLayoutState extends State<MainLayout> {
  // ...

  final List<Widget> _screens = <Widget>[
    BlocProvider.value(
      value: widget.homeBloc,
      child: const HomeScreen(),
    ),
    // ...
  ];

  // ...
}

This way, you can create the HomeBloc once and reuse it, potentially improving performance.

lib/main.dart (1)

Potential Issue with dotenv.load()

The dotenv.load() function is called in lib/main.dart, but no .env files were found in the repository. Please verify whether environment variables are managed differently or if the dotenv.load() call is necessary.

🔗 Analysis chain

Line range hint 16-26: Approve changes, but clarification needed

The addition of dotenv.load() is a good practice for configuration management. However, could you please clarify how this relates to the GPS address selector functionality mentioned in the PR objectives? It would be helpful to understand the purpose of loading environment variables in this context.

To verify the usage of environment variables, let's check for any .env files in the project:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for .env files and their content related to GPS or address functionality

# Test: Search for .env files
fd -H '\.env' -x echo "Found .env file: {}"

# Test: Check .env files for GPS or address related variables
fd -H '\.env' -x grep -i -E 'gps|address|location' {} || echo "No GPS or address related variables found in .env files"

Length of output: 812


Script:

#!/bin/bash
# Description: Check for .env files and their content related to GPS or address functionality

# Search for .env files
fd -H '\.env$' -x echo "Found .env file: {}"

# Check .env files for GPS or address related variables
fd -H '\.env$' -x grep -i -E 'gps|address|location' {} || echo "No GPS or address related variables found in .env files"

Length of output: 105


Script:

#!/bin/bash
# Description: Check for .env files and their content related to GPS or address functionality

# Search for .env files
echo "Searching for .env files..."
env_files=$(fd -H '\.env$')

if [ -z "$env_files" ]; then
  echo "No .env files found in the repository."
else
  echo "Found .env file(s):"
  echo "$env_files"

  # Check .env files for GPS or address related variables
  echo "Checking for GPS or address related variables in .env files..."
  grep -i -E 'gps|address|location' $env_files || echo "No GPS or address related variables found in .env files."
fi

Length of output: 198


Script:

#!/bin/bash
# Description: Search for dotenv imports and usages in the Dart codebase

echo "Searching for 'dotenv' imports in Dart files..."
rg --type dart 'import .*dotenv' || echo "No dotenv imports found in Dart files."

echo "Searching for 'dotenv.load()' usages in Dart files..."
rg --type dart 'dotenv\.load\(\)' || echo "No dotenv.load() usages found in Dart files."

Length of output: 523

pubspec.yaml (1)

Line range hint 1-71: Consider updating and pinning dependency versions

While not directly related to the changes in this PR, I noticed that your project uses a wide range of dependencies. To ensure better stability and reproducibility, consider the following suggestions:

  1. Regularly update your dependencies to their latest compatible versions to benefit from bug fixes and new features.
  2. Pin your dependencies to specific versions rather than using the caret (^) syntax for critical dependencies. This ensures consistent builds across different environments.
  3. Consider using a dependency management tool or script to help keep your dependencies up-to-date and detect potential conflicts.

Here's an example of how you might pin a critical dependency:

dependencies:
  firebase_core: 3.6.0  # Pinned to a specific version

You can use the flutter pub outdated command to check for updates to your dependencies, and flutter pub upgrade to update them.

Also applies to: 72-108

lib/features/auth/presentation/bloc/auth_bloc.dart (7)

24-35: LGTM: Improved formatting and added user ID storage

The changes enhance readability and implement user ID storage upon successful sign-in.

Consider adding a comment explaining the purpose of storing the user ID for better code documentation.


50-61: LGTM: Consistent changes with _onSignInRequested

The changes to _onSignUpRequested mirror those in _onSignInRequested, improving readability and implementing user ID storage upon successful sign-up.

For consistency, consider adding a similar comment about storing the user ID as suggested for _onSignInRequested.


76-85: LGTM: Consistent changes with other authentication methods

The changes to _onSignUpWithGoogleRequested are consistent with the other authentication methods, improving readability and implementing user ID storage upon successful Google sign-up.

For consistency across all authentication methods, consider adding a comment about storing the user ID.


100-109: LGTM: Consistent changes with other authentication methods

The changes to _onSignInWithGoogleRequested maintain consistency with the other authentication methods, improving readability and implementing user ID storage upon successful Google sign-in.

For consistency across all authentication methods, consider adding a comment about storing the user ID.


124-131: LGTM: Improved formatting and added user ID removal

The changes enhance readability and implement user ID removal upon sign-out, which is consistent with the new user ID storage functionality.

Consider adding a comment explaining the purpose of removing the user ID for better code documentation.


138-146: LGTM: New methods for managing user ID storage

The new _saveUserId and _removeUserId methods appropriately implement the functionality to store and remove user IDs using SharedPreferences.

Consider the following improvements:

  1. Add comments explaining the purpose of each method.
  2. Consider using a constant for the 'userId' key to avoid potential typos and improve maintainability.
  3. In _removeUserId, consider using clear() instead of remove() if 'userId' is the only key you're storing.

Example:

const String _userIdKey = 'userId';

Future<void> _saveUserId(String userId) async {
  // Store the user ID for persistent auth state
  final SharedPreferences prefs = await SharedPreferences.getInstance();
  await prefs.setString(_userIdKey, userId);
}

Future<void> _removeUserId() async {
  // Clear the stored user ID on sign-out
  final SharedPreferences prefs = await SharedPreferences.getInstance();
  await prefs.clear();
}

Line range hint 1-146: Overall assessment: Improved authentication state management

The changes to AuthBloc consistently implement user ID storage and removal across all authentication methods, enhancing the app's ability to maintain authentication state. The code readability has been improved through consistent formatting of method signatures.

Consider the following architectural improvements:

  1. Extract the SharedPreferences logic into a separate service class (e.g., UserPreferences) to adhere to the Single Responsibility Principle and make it easier to test and potentially swap out the storage mechanism in the future.
  2. Use dependency injection for the new UserPreferences service to improve testability and flexibility.
  3. Consider implementing a mechanism to refresh the authentication token if it expires, which might involve storing additional information like token expiration time.

These changes will further improve the maintainability and scalability of the authentication system.

lib/features/home/presentation/screens/home_screen.dart (1)

70-75: Consider handling returned data from AddressScreen

After navigating to the AddressScreen, any data returned is not currently captured. If the AddressScreen can return updated address information or other relevant data, consider awaiting and handling the result to update the UI accordingly.

Modify the navigation code if needed:

final result = await Navigator.push(
  context,
  MaterialPageRoute<void>(
    builder: (BuildContext context) => const AddressScreen(),
  ),
);
// Handle 'result' if necessary
lib/features/address/presentation/screens/address_screen.dart (3)

26-27: Consider initializing the TextEditingController in initState

While declaring _deliveryDetailsController as a final variable is acceptable, initializing it within initState() is a common practice for better lifecycle management.

Apply this diff to move initialization to initState():

-final TextEditingController _deliveryDetailsController = TextEditingController();
+late final TextEditingController _deliveryDetailsController;

@override
void initState() {
  super.initState();
+ _deliveryDetailsController = TextEditingController();
  _getCurrentLocation();
}

138-140: Add tooltip to FloatingActionButton for better accessibility

Consider adding a tooltip to the FloatingActionButton to improve accessibility for users relying on assistive technologies.

Apply this diff to add a tooltip:

FloatingActionButton(
  onPressed: _onMyLocationButtonPressed,
+ tooltip: 'My Location',
  child: const Icon(Icons.my_location),
),

185-192: Consider adding input constraints to the TextField

While the delivery details are optional, adding input constraints like a maximum length or input format can enhance data consistency and user experience.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 030ae2e and 0348222.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • .gitignore (1 hunks)
  • .vscode/settings.json (1 hunks)
  • lib/core/ui/layouts/main_layout.dart (3 hunks)
  • lib/core/utils/user_util.dart (1 hunks)
  • lib/features/address/domain/address.dart (0 hunks)
  • lib/features/address/domain/models/address.dart (1 hunks)
  • lib/features/address/presentation/bloc/address_bloc.dart (1 hunks)
  • lib/features/address/presentation/bloc/address_event.dart (1 hunks)
  • lib/features/address/presentation/bloc/address_state.dart (2 hunks)
  • lib/features/address/presentation/screens/address_detail_screen.dart (0 hunks)
  • lib/features/address/presentation/screens/address_screen.dart (5 hunks)
  • lib/features/address/repository/address_repository.dart (1 hunks)
  • lib/features/auth/presentation/bloc/auth_bloc.dart (5 hunks)
  • lib/features/home/presentation/screens/home_screen.dart (4 hunks)
  • lib/main.dart (4 hunks)
  • pubspec.yaml (1 hunks)
💤 Files with no reviewable changes (2)
  • lib/features/address/domain/address.dart
  • lib/features/address/presentation/screens/address_detail_screen.dart
🔇 Additional comments (35)
lib/core/utils/user_util.dart (1)

1-1: LGTM: Correct import statement

The import statement for the shared_preferences package is correct and necessary for the function's implementation.

lib/features/address/presentation/bloc/address_event.dart (2)

12-12: New ClearAddress event added, verify implementation.

The addition of the ClearAddress event is a good improvement, allowing for address data to be cleared or reset.

Let's verify the implementation of this new functionality:

#!/bin/bash
# Description: Check for ClearAddress usage and related methods

# Search for ClearAddress usage
echo "Searching for ClearAddress usage:"
rg --type dart 'ClearAddress'

# Search for methods related to clearing addresses
echo "Searching for methods related to clearing addresses:"
rg --type dart 'clear.*ddress'

10-10: LoadAddress class simplified, verify intended functionality.

The renaming and simplification of the LoadAddress class suggest a change in functionality. The empty class implies that no additional data is needed to trigger the address loading action.

Let's verify the usage of this event and ensure it aligns with the intended functionality:

lib/features/address/presentation/bloc/address_state.dart (3)

Line range hint 1-18: Verify the impact of removing the AddressSaved state.

The AddressSaved class has been removed from the state definitions. This could potentially impact the overall flow of the address-related functionality in the app.

Please run the following script to ensure that all references to AddressSaved have been removed or replaced appropriately:

#!/bin/bash
# Description: Find any remaining references to AddressSaved in the codebase

rg --type dart "AddressSaved"

If any occurrences are found, please update them to reflect the new state structure. Could you provide more context on how the removal of this state contributes to improving the GPS address selector, as mentioned in the PR objective?


10-11: ⚠️ Potential issue

Significant change in AddressLoaded state representation.

The AddressLoaded state has been modified to hold a single Address object instead of a list of addresses. This change could significantly impact how the address selection process works in the app.

Please ensure that this change is reflected in all places where AddressLoaded is used. Run the following script to find all occurrences of AddressLoaded:

#!/bin/bash
# Description: Find all occurrences of AddressLoaded in the codebase

rg --type dart "AddressLoaded"

Review each occurrence to ensure it's updated to work with a single address instead of a list. This change might be related to improving the GPS address selector as mentioned in the PR objective. Could you provide more context on how this change improves the address selection process?


1-1: Verify the consistency of the import change across the codebase.

The import statement has been updated from domain/address.dart to models/address.dart. This change suggests a restructuring of the project architecture.

Please run the following script to ensure this change is consistent across the entire codebase:

If any inconsistencies are found, please update the remaining files accordingly.

.gitignore (2)

45-45: Excellent addition of .env to .gitignore

Adding .env to the .gitignore file is a good practice. It prevents sensitive information, such as API keys or database credentials, from being accidentally committed to the repository.


45-45: Clarify the relationship between this change and the PR objectives

The addition of .env to .gitignore is a good practice, but it's not immediately clear how this change relates to fixing or improving the GPS address selector (as mentioned in the PR title "Fix/better gps address selector #36").

Could you please provide more context on how this change contributes to addressing the issue? Are there additional files or changes that should be part of this PR to fully implement the fix for the GPS address selector?

To help understand the full scope of changes related to this PR, you can run the following command to search for other modified files that might be relevant:

This will help identify any other files that might be part of this PR and related to the GPS address selector functionality.

✅ Verification successful

The .gitignore change does not impact the GPS address selector functionality

The addition of .env to .gitignore is a good practice, but it doesn't directly relate to fixing or improving the GPS address selector as mentioned in the PR title "Fix/better gps address selector #36".

Please ensure that all changes related to the GPS address selector are included in this PR to fully implement the intended fixes or improvements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for files that might be related to GPS address selector functionality
git diff --name-only HEAD~1 | grep -iE 'gps|address|location|map'

Length of output: 67

lib/features/address/repository/address_repository.dart (2)

12-19: LGTM! Improved code formatting.

The reformatting of the fetchUserAddresses method enhances readability without altering its functionality. The new structure aligns well with Dart style guidelines.


2-2: LGTM! Verify other import statements in the project.

The update to the import path for the Address model is consistent with best practices for organizing domain models. This change improves the project structure.

To ensure consistency across the project, please run the following script to check for any other files that might need to update their import statements:

✅ Verification successful

Verified! No other files are importing the old path for the Address model.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find other files that might need to update their import statements for the Address model.

# Test: Search for old import statements of the Address model
rg --type dart "import.*features/address/domain/address\.dart"

Length of output: 64

lib/features/address/domain/models/address.dart (3)

1-7: LGTM: Well-structured class definition and constructor.

The Address class is well-defined with a clear constructor. The use of required for mandatory fields and allowing an optional deliveryDetails provides both structure and flexibility.


19-22: LGTM: Well-defined immutable properties.

The class properties are correctly defined as final, ensuring immutability. The nullable deliveryDetails aligns with its optional nature in the constructor. This design promotes a robust and predictable state for Address objects.


1-33: Overall, good implementation with room for minor improvements.

This Address class provides a robust structure for managing address data, which aligns well with the PR objective of improving the GPS address selector (#36). The immutable design and clear serialization methods are commendable.

To further enhance this implementation:

  1. Strengthen input validation in the fromMap method to ensure data integrity.
  2. Consider using Dart's shorthand syntax in the toMap method for improved readability.

These changes will contribute to a more reliable and maintainable GPS address selector functionality.

lib/core/ui/layouts/main_layout.dart (3)

3-3: LGTM: New imports added correctly.

The new imports for HomeBloc and flutter_bloc are correctly added and necessary for the changes made in the _screens list.

Also applies to: 9-9


44-46: LGTM: Improved formatting for SystemUiOverlayStyle.

The changes to the SystemUiOverlayStyle setup improve code readability by breaking long lines into multiple lines. The logic remains unchanged, and this formatting enhancement contributes to better code maintainability.

Also applies to: 48-50


Line range hint 1-62: Request for more information on GPS address selector improvements.

The changes in this file introduce BLoC pattern for the HomeScreen and improve code formatting, which are positive improvements. However, the PR description mentions fixing the GPS address selector (issue #36), but there are no visible changes related to this in the current file.

Could you please provide more information about the specific improvements made to the GPS address selector? Are there changes in other files that address this issue?

To help verify the changes, please run the following script:

This will help us locate the relevant changes and ensure that the GPS address selector improvements are properly implemented.

✅ Verification successful

GPS Address Selector Improvements Verified

The GPS address selector enhancements are implemented across multiple files in this PR:

  • lib/features/address/presentation/screens/address_screen.dart
  • lib/features/address/repository/address_repository.dart
  • lib/features/address/presentation/bloc/address_bloc.dart
  • lib/features/auth/presentation/bloc/auth_bloc.dart

These changes collectively address the improvements mentioned in the PR description.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for GPS or address-related changes in the codebase

# Search for GPS or address-related changes
echo "Searching for GPS or address-related changes:"
rg -i -t dart "(gps|address|location|geocod)" --glob "!lib/core/ui/layouts/main_layout.dart"

# Search for files modified in this PR
echo -e "\nFiles modified in this PR:"
git diff --name-only origin/main

Length of output: 16092

lib/main.dart (3)

2-2: LGTM: AddressBloc import added

The import for AddressBloc is correctly added and aligns with the PR objective of improving the GPS address selector functionality.


Line range hint 1-67: Overall changes look good, but some clarifications needed

The changes in this file generally align with the PR objective of improving the GPS address selector functionality. The main points to address are:

  1. Clarify the purpose of loading environment variables in relation to the GPS address selector functionality.
  2. Verify if AddressBloc requires any initialization parameters.

Once these points are addressed, the changes should be ready for approval.


60-62: AddressBloc integration looks good, but verify initialization

The addition of AddressBloc to the MultiBlocProvider is appropriate and aligns with the PR objective. However, please verify if AddressBloc requires any initialization parameters. Currently, it's being created without any arguments, which might be insufficient for proper functionality.

Let's check the AddressBloc implementation to see if it requires any parameters:

#!/bin/bash
# Description: Check AddressBloc implementation for required parameters

# Test: Search for AddressBloc class definition
ast-grep --lang dart --pattern $'class AddressBloc extends Bloc<$_, $_> {
  $$$
  AddressBloc($_) $$$
  $$$
}'

# Test: If not found, search for any AddressBloc mentions
rg --type dart -A 5 'class AddressBloc'
pubspec.yaml (1)

71-71: ⚠️ Potential issue

Security concern: Reconsider adding .env to assets

Adding the .env file to the assets bundle raises significant security and maintainability concerns:

  1. Security risk: Sensitive information in the .env file will be bundled with the app and potentially accessible to users through reverse engineering.
  2. Version control: .env files typically contain environment-specific configurations and are usually not committed to version control.
  3. Reduced flexibility: Bundling .env makes it harder to change configurations without rebuilding the app.

Consider the following alternatives:

  1. Use build-time environment variables instead of runtime .env files. This can be achieved using --dart-define flags during the build process.
  2. If runtime configuration is necessary, consider using a secure, remote configuration service instead of bundling sensitive data with the app.
  3. If you must use a local .env file, ensure it's loaded at runtime from a secure location on the device, not bundled with the app.

Here's how you can use build-time environment variables:

  1. Remove the .env file from assets:
flutter:
  assets:
    - assets/
    - assets/carousel/
-   - .env
  1. Use --dart-define when building or running your app:
flutter run --dart-define=API_KEY=your_api_key --dart-define=OTHER_CONFIG=other_value
  1. Access these values in your Dart code:
const apiKey = String.fromEnvironment('API_KEY');
const otherConfig = String.fromEnvironment('OTHER_CONFIG');

This approach is more secure and flexible, allowing different configurations for different environments without rebuilding the app.

To ensure no sensitive information is being committed, run the following command:

lib/features/auth/presentation/bloc/auth_bloc.dart (2)

6-6: LGTM: Addition of shared_preferences package

The import of the shared_preferences package is appropriate for implementing persistent storage of user IDs.


12-17: Improved code formatting

The reformatting of event handler registrations for Google sign-up and sign-in enhances code readability without altering functionality.

lib/features/address/presentation/bloc/address_bloc.dart (2)

8-9: Confirm the correct handling of new events

The event handlers have been updated to include LoadAddress and ClearAddress. Ensure that these events are properly defined and that they integrate seamlessly with the bloc's logic.


6-6: Verify the removal of the repository dependency

The constructor AddressBloc() no longer accepts a repository parameter. Ensure that all repository interactions have been refactored appropriately and that the bloc functions correctly without the repository.

Run the following script to check for any remaining usages of repository within the AddressBloc:

lib/features/address/presentation/screens/address_screen.dart (11)

4-5: Imports are properly added for Address management and BLoC

The added imports for Address and AddressBloc are necessary for handling address data and state management.


8-9: Imports for state management and environment variables

The imports for flutter_bloc and flutter_dotenv are correctly added, enabling BLoC pattern implementation and access to environment variables.


32-33: Calling _getCurrentLocation in initState ensures timely location fetch

Initiating _getCurrentLocation() in initState() is appropriate to fetch the user's location as soon as the screen is loaded.


35-38: Proper disposal of TextEditingController

Disposing _deliveryDetailsController in dispose() prevents memory leaks and is a good practice.


75-76: Desired accuracy set appropriately for location fetching

Using LocationAccuracy.high ensures that the fetched location is precise, which is important for address selection.


100-104: CustomAppBar configured with appropriate title and back button

The CustomAppBar is correctly set up, enhancing the app's navigation.


106-133: GoogleMap widget properly integrated within a Stack

Wrapping the GoogleMap in a Stack allows for overlaying additional widgets like the floating action button and bottom sheet.


143-143: Adding _buildBottomSheet enhances user interaction

Including _buildBottomSheet() in the widget tree provides a clear interface for users to confirm their address and add delivery details.


149-213: _buildBottomSheet is well-structured and user-friendly

The bottom sheet UI is effectively designed, enhancing the overall user experience with intuitive components.


194-207: Confirm Address button logic is correctly implemented

The ElevatedButton enables only when selectedPosition is not null, and the address is correctly constructed and saved.


272-272: Securely accessing the Google Maps API key

Retrieving the API key from environment variables using dotenv enhances security by avoiding hardcoding sensitive information.

Comment thread lib/features/address/presentation/bloc/address_event.dart
Comment thread lib/features/address/domain/models/address.dart
Comment thread lib/features/address/presentation/bloc/address_bloc.dart
Comment thread lib/features/address/presentation/bloc/address_bloc.dart
Comment thread lib/features/address/presentation/bloc/address_bloc.dart
Comment thread lib/features/home/presentation/screens/home_screen.dart
Comment thread lib/features/home/presentation/screens/home_screen.dart
Comment thread lib/features/address/presentation/screens/address_screen.dart
Comment thread lib/features/address/presentation/screens/address_screen.dart
Copy link
Copy Markdown
Contributor

@RaulRinconX RaulRinconX left a comment

Choose a reason for hiding this comment

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

Good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

Status: DONE

Development

Successfully merging this pull request may close these issues.

fix/gps-googleMaps-do-not-save-location

2 participants