Skip to content

Improve app navigation. #111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amaury901130 merged 5 commits into from
Nov 17, 2025
Merged

Improve app navigation. #111

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e383801
improve app navigation, and save navigation based on the auth state
amaury901130 Nov 12, 2025
b10b72b
improve auth state ref
amaury901130 Nov 12, 2025
394ee16
imrpove go router
amaury901130 Nov 17, 2025
15f9331
add deeplink support
amaury901130 Nov 17, 2025
24a3a95
add deeplink support
amaury901130 Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 9 additions & 20 deletions app/lib/main/app.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:app/presentation/ui/custom/loading_screen.dart';
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
import 'package:common/core/resource.dart';
import 'package:flutter/material.dart';
import 'package:domain/bloc/app/app_cubit.dart';
Expand All @@ -10,13 +11,10 @@ import 'package:app/presentation/themes/app_themes.dart';
import 'package:app/presentation/utils/lang_extensions.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:flutter_localizations/flutter_localizations.dart';
import 'package:go_router/go_router.dart';

import 'init.dart';

class App extends StatelessWidget {
GoRouter get _goRouter => Routers.authRouter;

const App({super.key});

@override
Expand All @@ -38,23 +36,14 @@ class App extends StatelessWidget {
GlobalWidgetsLocalizations.delegate,
GlobalCupertinoLocalizations.delegate,
],
builder: (context, child) {
return BlocListener<AuthCubit, Resource>(
listener: (_, state) {
if (state is RSuccess<AuthState>) {
switch (state.data) {
case AuthStateAuthenticated _:
_goRouter.go('/home');
case AuthStateUnauthenticated _:
_goRouter.go('/login');
case _:
}
}
},
child: child,
);
},
routerConfig: _goRouter,
builder: (context, child) =>
child ??
const Material(
child: Center(
child: CircularProgressIndicator(),
),
),
routerConfig: Routes.router,
);
},
),
Expand Down
5 changes: 3 additions & 2 deletions app/lib/main/init.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ import 'package:data/init.dart';
import 'package:domain/init.dart';
import 'package:flutter/material.dart';
import 'package:get_it/get_it.dart';
import 'package:url_strategy/url_strategy.dart';
import 'package:flutter_web_plugins/flutter_web_plugins.dart';


void init() async {
WidgetsFlutterBinding.ensureInitialized();
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The usePathUrlStrategy() call should be placed after WidgetsFlutterBinding.ensureInitialized() but before any other initialization. However, it's currently placed after the binding initialization but on a separate line without a comment. While functionally correct, consider adding a comment explaining why path-based URLs are being used instead of hash-based URLs (e.g., for better SEO, cleaner URLs).

Suggested change
WidgetsFlutterBinding.ensureInitialized();
WidgetsFlutterBinding.ensureInitialized();
// Use path-based URLs instead of hash-based URLs for better SEO and cleaner URLs.

Copilot uses AI. Check for mistakes.
usePathUrlStrategy();
await initialize();
setHashUrlStrategy();
runApp(const App());
}

Expand Down
108 changes: 89 additions & 19 deletions app/lib/presentation/navigation/routers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,102 @@
import 'package:app/main/init.dart';
import 'package:app/presentation/ui/pages/home/home_page.dart';
import 'package:app/presentation/ui/pages/login/login_page.dart';
import 'package:app/presentation/ui/pages/sign_up/sign_up_page.dart';
import 'package:app/presentation/ui/pages/splash/splash_page.dart';
import 'package:common/core/resource.dart';
import 'package:domain/bloc/auth/auth_cubit.dart';
import 'package:domain/bloc/auth/auth_state.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:go_router/go_router.dart';

enum Routes {
auth,
login,
signUp,
app,
home;

String get path => '/$name';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

[nitpick] The path getter returns '/$name' which assumes the route is always at the root level. This creates inconsistency when used in nested route structures (as seen in the usage on lines 59 and 83). Consider renaming this to rootPath to make its intent clear, and use subPath for nested routes.

Copilot uses AI. Check for mistakes.
String get subPath => name;

void nav({Object? extra}) {
Routers.authRouter.goNamed(
name,
extra: extra,
);
}
Comment thread
amaury901130 marked this conversation as resolved.
Outdated

static GoRouter get router => Routers.authRouter;
}
Comment on lines +13 to +37
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The Routes enum lacks documentation explaining its purpose and how it should be used. Given its central role in the navigation architecture, consider adding documentation:

  • What each route represents
  • When to use path vs subPath
  • How the nav() method should be used
  • The relationship with the Routers class

This is especially important since this appears to be a new architectural pattern for the project.

Copilot uses AI. Check for mistakes.

class Routers {
static GoRouter authRouter = GoRouter(
initialLocation: "/splash",
initialLocation: '/',
routes: [
GoRoute(
name: "login",
path: "/login",
builder: (context, state) => const LoginPage(),
),
GoRoute(
name: "splash",
path: "/splash",
builder: (context, state) => const SplashPage(),
),
GoRoute(
name: "signUp",
path: "/signUp",
builder: (context, state) => const SignUpPage(),
),
GoRoute(
name: "home",
path: "/home",
builder: (context, state) => const HomePage(),
path: '/',
builder: (context, state) {
return BlocListener<AuthCubit, Resource>(
listener: (_, state) {
if (state is RSuccess) {
switch (state.data) {
case AuthStateAuthenticated _:
Routes.app.nav();
break;
case AuthStateUnauthenticated _:
Routes.auth.nav();
break;
case _:
}
}
},
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
child: const SplashPage(instant: true),
);
},
routes: [
GoRoute(
name: Routes.auth.name,
path: Routes.auth.path,
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The nested route structure has a critical issue: the auth route uses path: Routes.auth.path which evaluates to '/auth', but since it's a child route of the root '/', the full path becomes '//auth' (double slash). Child routes in go_router should use relative paths without the leading slash.

Change path: Routes.auth.path to path: Routes.auth.subPath (or just 'auth') to fix the path construction.

Suggested change
path: Routes.auth.path,
path: Routes.auth.subPath,

Copilot uses AI. Check for mistakes.
redirect: (context, state) {
if (getIt<AuthCubit>().isLoggedIn()) {
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
return '${Routes.app.path}${Routes.home.path}';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The redirect logic has a critical issue: when accessing /auth route directly, it will be constructed with path '/$name' which evaluates to '/auth'. However, the redirect at line 62 returns '${Routes.app.path}${Routes.home.path}' which evaluates to '/app/home'. This creates inconsistent path construction - the auth route uses path while the redirect uses nested path concatenation. This may cause navigation issues.

Consider making the path construction consistent: either use Routes.app.path + '/' + Routes.home.subPath or restructure to avoid manual path concatenation.

Suggested change
return '${Routes.app.path}${Routes.home.path}';
return '${Routes.app.path}/${Routes.home.subPath}';

Copilot uses AI. Check for mistakes.
}

return '${Routes.auth.path}${Routes.login.path}';
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
},
routes: [
GoRoute(
name: Routes.login.name,
path: Routes.login.subPath,
builder: (context, state) => const LoginPage(),
),
GoRoute(
name: Routes.signUp.name,
path: Routes.signUp.subPath,
builder: (context, state) => const SignUpPage(),
),
],
),
GoRoute(
name: Routes.app.name,
path: Routes.app.path,
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The nested route path is incorrect. Since this is a child route under '/', the path should be relative (e.g., 'app') not absolute ('/app'). Using Routes.app.path which returns '/$name' will create the path '//app'. Change to use Routes.app.subPath or a relative path string 'app'.

Suggested change
path: Routes.app.path,
path: Routes.app.subPath,

Copilot uses AI. Check for mistakes.
builder: (context, state) => const SplashPage(),
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The builder for the /app route returns const SplashPage() without the instant parameter, which defaults to true based on line 7 of splash_page.dart. However, this causes the splash screen to call _authCubit.onValidate() immediately, which will trigger navigation back through the BlocListener at the root route. This creates a circular navigation pattern and unnecessary re-validation.

Consider removing this builder entirely or using a different widget, as the app route should only serve as a container for its child routes, not display a splash screen.

Suggested change
builder: (context, state) => const SplashPage(),
builder: (context, state) => const SizedBox.shrink(),

Copilot uses AI. Check for mistakes.
redirect: (context, state) {
if (!getIt<AuthCubit>().isLoggedIn()) {
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
return '${Routes.auth.path}${Routes.login.path}';
}

return '${Routes.app.path}${Routes.home.path}';
Comment thread
amaury901130 marked this conversation as resolved.
Outdated
},
routes: [
GoRoute(
name: Routes.home.name,
path: Routes.home.subPath,
builder: (context, state) => const HomePage(),
),
],
),
],
),
],
);
Expand Down
2 changes: 2 additions & 0 deletions app/lib/presentation/ui/pages/home/home_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class HomeView extends StatelessWidget {
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: const Text('Home'),
automaticallyImplyLeading: false,
actions: [
IconButton(
onPressed: () => _authCubit.logOut(),
Expand Down
1 change: 0 additions & 1 deletion app/lib/presentation/ui/pages/login/login_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import 'package:app/main/init.dart';
import 'package:app/presentation/resources/resources.dart';
import 'package:app/presentation/themes/app_themes.dart';
import 'package:app/presentation/ui/custom/app_theme_switch.dart';
import 'package:app/presentation/ui/custom/loading_screen.dart';
import 'package:common/core/resource.dart';
Expand Down
5 changes: 3 additions & 2 deletions app/lib/presentation/ui/pages/splash/splash_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import 'package:domain/bloc/auth/auth_cubit.dart';
import 'package:flutter/material.dart';

class SplashPage extends StatefulWidget {
Comment thread
amaury901130 marked this conversation as resolved.
const SplashPage({super.key});
final bool instant;
const SplashPage({super.key, this.instant = true});

@override
State<SplashPage> createState() => _SplashPageState();
Expand All @@ -20,7 +21,7 @@ class _SplashPageState extends State<SplashPage> {

/// Add post frame callback to avoid calling bloc methods during build
WidgetsBinding.instance.addPostFrameCallback((_) async {
await Future.delayed(const Duration(seconds: 1));
await Future.delayed(Duration(seconds: widget.instant ? 0 : 2));
_authCubit.onValidate();
});
}
Expand Down
7 changes: 5 additions & 2 deletions app/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ publish_to: "none" # Remove this line if you wish to publish to pub.dev
version: 1.0.0+1

environment:
sdk: '>=3.0.0 <4.0.0'
sdk: ">=3.0.0 <4.0.0"

dependencies:
flutter:
Expand All @@ -35,7 +35,7 @@ dependencies:
package_info_plus: ^8.3.0
permission_handler: ^11.4.0
universal_html: ^2.2.2
go_router: ^7.0.1
go_router: ^17.0.0
equatable: ^2.0.5
firebase_core: ^3.13.0
url_strategy: ^0.2.0
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The url_strategy package is no longer being used in the code since it was replaced with flutter_web_plugins in init.dart. This dependency should be removed from pubspec.yaml to avoid including unused packages.

Suggested change
url_strategy: ^0.2.0

Copilot uses AI. Check for mistakes.
Expand All @@ -51,6 +51,9 @@ dependencies:
http: ^1.5.0
melos: ^3.4.0
dev: ^1.0.0
flutter_dotenv: ^5.2.1
flutter_web_plugins:
sdk: flutter

dev_dependencies:
bloc_test: ^9.0.2
Expand Down
3 changes: 3 additions & 0 deletions modules/domain/lib/bloc/auth/auth_cubit.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ class AuthCubit extends BaseCubit<AuthState> {
isLogOut();
}

Comment thread
amaury901130 marked this conversation as resolved.
bool isLoggedIn() =>
state is RSuccess && (state as RSuccess).data is AuthStateAuthenticated;
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The isLoggedIn() method performs runtime type checking with state as RSuccess without first verifying the cast will succeed. While the first condition state is RSuccess protects against null/wrong type, this pattern could be simplified. Consider using pattern matching or extracting the data check:

bool isLoggedIn() {
  return state case RSuccess(data: AuthStateAuthenticated()) => true, _ => false;
}

Or using a safer approach:

bool isLoggedIn() {
  if (state is! RSuccess) return false;
  return (state as RSuccess).data is AuthStateAuthenticated;
}
Suggested change
bool isLoggedIn() =>
state is RSuccess && (state as RSuccess).data is AuthStateAuthenticated;
// Use pattern matching for safer and more concise type checking.
bool isLoggedIn() {
return state case RSuccess(data: AuthStateAuthenticated()) => true, _ => false;
}

Copilot uses AI. Check for mistakes.

void isLogin() => isSuccess(AuthStateAuthenticated());

void isLogOut() => isSuccess(AuthStateUnauthenticated());
Expand Down