Navigate to homescreen only via go router #1289
Conversation
WalkthroughThe pull request introduces changes to the navigation logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/features/family/features/reflect/presentation/widgets/leave_game_dialog.dart (1)
27-29: Consider adding error handlingThe navigation logic could be more robust with proper error handling:
onTap: () { - getIt<SummaryCubit>().saveSummary(); - context.goNamed( - FamilyPages.profileSelection.name, - ); + try { + getIt<SummaryCubit>().saveSummary(); + context.goNamed( + FamilyPages.profileSelection.name, + ); + } catch (e) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Failed to leave game. Please try again.')), + ); + } },lib/features/family/features/reflect/presentation/pages/summary_screen.dart (1)
84-85: Remove or clarify the "hmm" commentThe comment appears to be a placeholder and doesn't provide any meaningful information. Consider removing it or replacing it with actual documentation if there's something that needs to be noted.
- //hmm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/features/family/features/reflect/presentation/pages/summary_screen.dart(3 hunks)lib/features/family/features/reflect/presentation/widgets/leave_game_dialog.dart(2 hunks)
🔇 Additional comments (4)
lib/features/family/features/reflect/presentation/widgets/leave_game_dialog.dart (2)
10-10: LGTM: Import added for Go Router integration
The go_router import is correctly added to support the new navigation approach.
27-29: Verify state cleanup before navigation
While the navigation change aligns with the PR objective, there are a few considerations:
- The
goNamedcall will clear the entire navigation stack. Verify this is the intended behavior. - Consider adding error handling for both
saveSummary()and navigation.
Let's verify the state cleanup requirements:
✅ Verification successful
Based on the search results, I can generate the final response now:
Navigation pattern is consistent with app-wide usage
The goNamed navigation to profile selection is used consistently across the app in similar scenarios (splash, registration, notifications, etc.) without explicit state cleanup. Since this is a StatelessWidget and the navigation pattern follows established conventions in the codebase, no additional cleanup is required.
- The
saveSummary()call before navigation ensures data persistence goNamedclearing the navigation stack is the intended behavior, as evidenced by its consistent usage across similar flows- Error handling can be considered but isn't strictly required given the established patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any state cleanup patterns in similar navigation scenarios
rg -A 5 "goNamed.*profileSelection"
# Check for any dispose or cleanup methods that might be relevant
ast-grep --pattern 'class $_ extends StatelessWidget {
$$$
dispose() {
$$$
}
$$$
}'
Length of output: 4674
lib/features/family/features/reflect/presentation/pages/summary_screen.dart (2)
5-5: LGTM: Required imports added for Go Router implementation
The new imports align with the PR objective to use Go Router for navigation.
Also applies to: 15-15
89-91: LGTM: Navigation properly implemented using Go Router
The implementation correctly uses named routes with Go Router, which is more maintainable than using string literals.
Let's verify that the route name is properly registered:
✅ Verification successful
LGTM: Route name is properly registered and consistently used
The verification confirms that:
- The route is properly registered in
lib/features/family/app/family_pages.dartasprofileSelectionwith path '/profile-selection' and name 'PROFILE_SELECTION' - The route is correctly configured in the router setup in
lib/features/family/app/family_routes.dart - The route name is consistently used across multiple features in the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the profileSelection route is properly registered in the router
# Expected: Find the route definition in app_router.dart
# Search for the route definition
ast-grep --pattern 'name: $route' | grep -i "profileSelection"
# Also check for any usages to ensure consistency
rg "profileSelection" --type dart
Length of output: 2779
| Navigator.of(context).popUntil( | ||
| ModalRoute.withName( | ||
| FamilyPages.profileSelection.name, | ||
| ), |
There was a problem hiding this comment.
Previous solution was clearing the stack first, is that not needed anymore?
I think it can cause other problems if we don't do that
There was a problem hiding this comment.
Somewhere else in the app we also have a clear solution for this navigation (with a for loop I think)
There was a problem hiding this comment.
There are only 2 places we navigate without gorouter to this specific path and this these. From what i find online goNamed clears the stack, but im trying to test
There was a problem hiding this comment.
Ah okay! Maybe that was with a push what I have seen :)
Did a search on Google: With go() "Navigating to a destination in GoRouter will replace the current stack of screens with the screens configured to be displayed for the destination route."
There was a problem hiding this comment.
I guess that is the same as named :)
There was a problem hiding this comment.
Navigating to a destination in GoRouter will replace the current stack of screens with the screens configured to be displayed for the destination route.
GoRouter docs
Summary by CodeRabbit
New Features
Bug Fixes