Logout functionality #2483
Logout functionality #2483
Conversation
📝 WalkthroughWalkthroughThis update introduces structured logout functionality in the authentication flow. The Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 1
🧹 Nitpick comments (1)
mobile-v3/lib/src/app/auth/bloc/auth_bloc.dart (1)
75-85: Consider enhancing logout cleanup process.While the basic logout functionality is implemented correctly, consider:
- Clearing all user-related data from Hive, not just the token
- Specifying error types in the catch block for more precise error handling
Future<void> _onLogoutUser(LogoutUser event, Emitter<AuthState> emit) async { emit(AuthLoading()); try { - await HiveRepository.deleteData( - 'token', HiveBoxNames.authBox); // Remove token from Hive + // Clear all user-related data + await Future.wait([ + HiveRepository.deleteData('token', HiveBoxNames.authBox), + HiveRepository.deleteData('user_preferences', HiveBoxNames.authBox), + // Add other user-related data cleanup + ]); emit(GuestUser()); // Emit guest state after logout - } catch (e) { + } catch (e) when (e is HiveError) { + debugPrint("Hive storage error during logout: $e"); + emit(AuthLoadingError("Storage error during logout. Please try again.")); + } catch (e) { debugPrint("Logout error: $e"); emit(AuthLoadingError("Failed to log out. Please try again.")); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mobile-v3/lib/src/app/auth/bloc/auth_bloc.dart(4 hunks)mobile-v3/lib/src/app/auth/bloc/auth_event.dart(1 hunks)mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart(3 hunks)
🔇 Additional comments (2)
mobile-v3/lib/src/app/auth/bloc/auth_event.dart (1)
30-32: Clean implementation of LogoutUser event!The implementation follows the established pattern and maintains consistency with other event classes.
mobile-v3/lib/src/app/auth/bloc/auth_bloc.dart (1)
22-22: LGTM! Event handler registration is clean.The LogoutUser event is properly registered in the constructor.
| onPressed: () { | ||
| // TODO: Implement actual logout logic | ||
| // e.g., clear user session, revoke tokens | ||
| Navigator.of(context).pushReplacementNamed('/login'); | ||
| context | ||
| .read<AuthBloc>() | ||
| .add(LogoutUser()); | ||
|
|
||
| Navigator.pushAndRemoveUntil( | ||
| context, | ||
| MaterialPageRoute(builder: (context) => WelcomeScreen()), | ||
| (Route<dynamic> route) => false, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state and handle logout completion.
The current implementation doesn't wait for the logout process to complete before navigation. This could lead to a poor user experience if the logout fails.
onPressed: () {
- context
- .read<AuthBloc>()
- .add(LogoutUser());
-
- Navigator.pushAndRemoveUntil(
- context,
- MaterialPageRoute(builder: (context) => WelcomeScreen()),
- (Route<dynamic> route) => false,
- );
+ // Show loading indicator
+ showDialog(
+ context: context,
+ barrierDismissible: false,
+ builder: (context) => const Center(child: CircularProgressIndicator()),
+ );
+
+ // Listen for state changes
+ context.read<AuthBloc>().stream.listen((state) {
+ if (state is GuestUser) {
+ Navigator.pushAndRemoveUntil(
+ context,
+ MaterialPageRoute(builder: (context) => WelcomeScreen()),
+ (Route<dynamic> route) => false,
+ );
+ } else if (state is AuthLoadingError) {
+ Navigator.pop(context); // Remove loading
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text(state.message)),
+ );
+ }
+ });
+
+ context.read<AuthBloc>().add(LogoutUser());
},📝 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.
| onPressed: () { | |
| // TODO: Implement actual logout logic | |
| // e.g., clear user session, revoke tokens | |
| Navigator.of(context).pushReplacementNamed('/login'); | |
| context | |
| .read<AuthBloc>() | |
| .add(LogoutUser()); | |
| Navigator.pushAndRemoveUntil( | |
| context, | |
| MaterialPageRoute(builder: (context) => WelcomeScreen()), | |
| (Route<dynamic> route) => false, | |
| ); | |
| }, | |
| onPressed: () { | |
| // Show loading indicator | |
| showDialog( | |
| context: context, | |
| barrierDismissible: false, | |
| builder: (context) => const Center(child: CircularProgressIndicator()), | |
| ); | |
| // Listen for state changes | |
| context.read<AuthBloc>().stream.listen((state) { | |
| if (state is GuestUser) { | |
| Navigator.pushAndRemoveUntil( | |
| context, | |
| MaterialPageRoute(builder: (context) => WelcomeScreen()), | |
| (Route<dynamic> route) => false, | |
| ); | |
| } else if (state is AuthLoadingError) { | |
| Navigator.pop(context); // Remove loading | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar(content: Text(state.message)), | |
| ); | |
| } | |
| }); | |
| context.read<AuthBloc>().add(LogoutUser()); | |
| }, |
…rs more gracefully
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart (2)
123-125: Consider implementing account deletion logic.The TODO comment indicates pending implementation. Consider creating a separate ticket to track this feature.
Would you like me to help create an issue for implementing the account deletion functionality?
157-157: Replace print statements with proper logging.Consider replacing
- print("Location setting: $value"); + log.debug("Location setting updated", {"enabled": value}); - print("Notifications setting: $value"); + log.debug("Notifications setting updated", {"enabled": value});Also applies to: 172-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart(3 hunks)
🔇 Additional comments (3)
mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart (3)
1-3: LGTM! Clean import additions.The new imports are well-organized and necessary for the auth functionality.
37-47: LGTM! Proper dialog context handling.Using
dialogContextinstead of the outercontextfor dialog operations prevents potential context conflicts.
54-89: Implementation follows suggested pattern.The logout implementation incorporates the previously suggested improvements, including:
- Loading indicator during logout
- Proper state handling for success and errors
- Error boundary for unexpected exceptions
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart(1 hunks)mobile-v3/lib/src/app/learn/pages/lesson_finished.dart(1 hunks)mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile-v3/lib/src/app/profile/pages/widgets/settings_widget.dart
🔇 Additional comments (3)
mobile-v3/lib/src/app/learn/pages/lesson_finished.dart (2)
17-17: Nice improvement to the message tone! 👍The change from "invite" to "teach" empowers users by recognizing their newly acquired knowledge and ability to educate others about air pollution.
21-29: Verify the removal of sharing functionality.The commented-out sharing and rating buttons seem unrelated to the PR's logout functionality objective. Consider:
- If these changes are intentional, they should be in a separate PR focused on UI modifications
- If temporary, using version control is preferred over commenting out code
Could you clarify if removing these social features is intentional and how it relates to the logout functionality?
mobile-v3/lib/src/app/dashboard/pages/dashboard_page.dart (1)
127-170: Well-structured auth state handling!The
BlocBuilderimplementation effectively manages different authentication states and provides appropriate avatar displays. The code:
- Handles guest users with a default avatar
- Shows user initials for authenticated users
- Includes proper loading states
| // onTap: () { | ||
| // final authState = context.read<AuthBloc>().state; | ||
| // if (authState is GuestUser) { | ||
|
|
||
| Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (context) => GuestProfilePage(), | ||
| ), | ||
| ); | ||
| } else { | ||
| // Navigate to the regular profile page | ||
| Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (context) => ProfilePage(), | ||
| ), | ||
| ); | ||
| } | ||
| }, | ||
| // Navigator.of(context).push( | ||
| // MaterialPageRoute( | ||
| // builder: (context) => GuestProfilePage(), | ||
| // ), | ||
| // ); | ||
| // } else { | ||
| // // Navigate to the regular profile page | ||
| // Navigator.of(context).push( | ||
| // MaterialPageRoute( | ||
| // builder: (context) => ProfilePage(), | ||
| // ), | ||
| // ); | ||
| // } | ||
| // }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code instead of keeping it.
The commented-out navigation logic should be removed rather than left as comments. If this code needs to be referenced later, it can be found in the git history.
Apply this diff to clean up the code:
- // onTap: () {
- // final authState = context.read<AuthBloc>().state;
- // if (authState is GuestUser) {
-
- // Navigator.of(context).push(
- // MaterialPageRoute(
- // builder: (context) => GuestProfilePage(),
- // ),
- // );
- // } else {
- // // Navigate to the regular profile page
- // Navigator.of(context).push(
- // MaterialPageRoute(
- // builder: (context) => ProfilePage(),
- // ),
- // );
- // }
- // },📝 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.
| // onTap: () { | |
| // final authState = context.read<AuthBloc>().state; | |
| // if (authState is GuestUser) { | |
| Navigator.of(context).push( | |
| MaterialPageRoute( | |
| builder: (context) => GuestProfilePage(), | |
| ), | |
| ); | |
| } else { | |
| // Navigate to the regular profile page | |
| Navigator.of(context).push( | |
| MaterialPageRoute( | |
| builder: (context) => ProfilePage(), | |
| ), | |
| ); | |
| } | |
| }, | |
| // Navigator.of(context).push( | |
| // MaterialPageRoute( | |
| // builder: (context) => GuestProfilePage(), | |
| // ), | |
| // ); | |
| // } else { | |
| // // Navigate to the regular profile page | |
| // Navigator.of(context).push( | |
| // MaterialPageRoute( | |
| // builder: (context) => ProfilePage(), | |
| // ), | |
| // ); | |
| // } | |
| // }, | |
| // (All the commented-out onTap navigation code has been removed) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mobile-v3/android/app/src/main/AndroidManifest.xml (1)
15-18: Addition of Google Places API MetadataA new metadata entry has been added for the Google Places API key using the placeholder
${PLACES_API_KEY}. Make sure that this placeholder is defined and managed (via gradle or properties files) similarly to the maps key to avoid runtime issues with unresolved references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mobile-v3/android/app/build.gradle(1 hunks)mobile-v3/android/app/src/main/AndroidManifest.xml(1 hunks)
🔇 Additional comments (3)
mobile-v3/android/app/src/main/AndroidManifest.xml (1)
12-13: Review of Google Maps API Metadata UpdateThe metadata entry for the Google Maps API key now references
${googleMapsKey}. This is a clear improvement if it aligns with the build script injection. Please double-check that your gradle configuration and manifest placeholders are consistently updated to use this new naming.mobile-v3/android/app/build.gradle (2)
42-45: Robust Fallback for Production Google Maps API KeyThe implementation that retrieves the production Google Maps API key using a fallback from the secrets file is robust. The error message clearly instructs where to define the key if missing. This helps prevent misconfiguration.
47-50: Robust Fallback for Development Google Maps API KeySimilarly, the fallback logic for the development API key improves the resiliency of key retrieval. The combined use of the properties file and secrets file is well-handled, and the error message provides precise guidance.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit