Skip to content

Fix Issue #369: Implement automatic login redirect after successful signup #371

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

Open
vnaik10 wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix Issue #369: Implement automatic login redirect after successful signup #371

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3db9b7
Fix Issue #369: Implement automatic login redirect after successful s…
vnaik10 May 31, 2025
e48ec8b
Resolve merge conflicts and align with Appwrite auth migration
vnaik10 Nov 18, 2025
299f049
Remove obsolete Firebase fromSnapshot factory
vnaik10 Nov 22, 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
Binary file modified .gitignore
View file
Open in desktop
Binary file not shown.
77 changes: 61 additions & 16 deletions lib/application/authentication/authentication_bloc.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,76 @@ class AuthenticationBloc
_mapAppStartedToState(
AuthenticationEvent event, Emitter<AuthenticationState> emit) async {
try {
final (userLoggedIn, user) = await _authRepository.getUser();
log('User: $userLoggedIn, $user');
if (userLoggedIn && user != null) {
emit(Authenticated(user.toEntity()));
} else if (userLoggedIn && user == null) {
emit(OnboardingIncomplete());
} else {
emit(Unauthenticated());

int retries = 0;
const maxRetries = 3;
const retryDelay = Duration(milliseconds: 300);

while (retries < maxRetries) {
final (userLoggedIn, user) = await _authRepository.getUser();
log('User: $userLoggedIn, $user (attempt ${retries + 1})');

if (userLoggedIn && user != null) {

emit(Authenticated(user.toEntity()));
return;
} else if (userLoggedIn && user == null) {
if (retries == maxRetries - 1) {

emit(OnboardingIncomplete());
return;
}
} else {

emit(Unauthenticated());
return;
}


retries++;
await Future.delayed(retryDelay);
}


emit(Unauthenticated());
Comment on lines +32 to +62
Copy link

@coderabbitai coderabbitai bot May 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve the retry mechanism implementation.

The retry logic is a good approach for handling eventual consistency issues, but there are opportunities for improvement:

  1. Code duplication: The retry logic is duplicated between _mapAppStartedToState and _mapLoggedInToState
  2. Magic numbers: Retry count and delay should be configurable constants
  3. Logging: Consider removing or making the logging conditional for production

Extract the retry logic into a private method:

+ static const int _maxRetries = 3;
+ static const Duration _retryDelay = Duration(milliseconds: 300);
+
+ Future<void> _handleAuthenticationCheck(Emitter<AuthenticationState> emit, {bool logAttempts = false}) async {
+   int retries = 0;
+   
+   while (retries < _maxRetries) {
+     final (userLoggedIn, user) = await _authRepository.getUser();
+     if (logAttempts) {
+       log('User: $userLoggedIn, $user (attempt ${retries + 1})');
+     }
+     
+     if (userLoggedIn && user != null) {
+       emit(Authenticated(user.toEntity()));
+       return;
+     } else if (userLoggedIn && user == null) {
+       if (retries == _maxRetries - 1) {
+         emit(OnboardingIncomplete());
+         return;
+       }
+     } else {
+       emit(Unauthenticated());
+       return;
+     }
+     
+     retries++;
+     await Future.delayed(_retryDelay);
+   }
+   
+   emit(Unauthenticated());
+ }

Then simplify both methods:

 _mapAppStartedToState(
     AuthenticationEvent event, Emitter<AuthenticationState> emit) async {
   try {
-    // ... existing retry logic
+    await _handleAuthenticationCheck(emit, logAttempts: true);
   } catch (_) {
     emit(Unauthenticated());
   }
 }
📝 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.

Suggested change
int retries = 0;
const maxRetries = 3;
const retryDelay = Duration(milliseconds: 300);
while (retries < maxRetries) {
final (userLoggedIn, user) = await _authRepository.getUser();
log('User: $userLoggedIn, $user (attempt ${retries + 1})');
if (userLoggedIn && user != null) {
emit(Authenticated(user.toEntity()));
return;
} else if (userLoggedIn && user == null) {
if (retries == maxRetries - 1) {
emit(OnboardingIncomplete());
return;
}
} else {
emit(Unauthenticated());
return;
}
retries++;
await Future.delayed(retryDelay);
}
emit(Unauthenticated());
// In AuthenticationBloc class
// Make retry settings configurable
static const int _maxRetries = 3;
static const Duration _retryDelay = Duration(milliseconds: 300);
// Extracted retry logic
Future<void> _handleAuthenticationCheck(
Emitter<AuthenticationState> emit, {
bool logAttempts = false,
}) async {
int retries = 0;
while (retries < _maxRetries) {
final (userLoggedIn, user) = await _authRepository.getUser();
if (logAttempts) {
log('User: $userLoggedIn, $user (attempt ${retries + 1})');
}
if (userLoggedIn && user != null) {
emit(Authenticated(user.toEntity()));
return;
} else if (userLoggedIn && user == null) {
if (retries == _maxRetries - 1) {
emit(OnboardingIncomplete());
return;
}
} else {
emit(Unauthenticated());
return;
}
retries++;
await Future.delayed(_retryDelay);
}
emit(Unauthenticated());
}
// Simplified mapping using the new helper
Future<void> _mapAppStartedToState(
AuthenticationEvent event,
Emitter<AuthenticationState> emit,
) async {
try {
await _handleAuthenticationCheck(emit, logAttempts: true);
} catch (_) {
emit(Unauthenticated());
}
}
🤖 Prompt for AI Agents 
In lib/application/authentication/authentication_bloc.dart around lines 32 to
62, the retry logic is duplicated and uses hardcoded retry count and delay
values, with unconditional logging. Refactor by extracting the retry mechanism
into a private method that accepts configurable retry count and delay
parameters, and includes optional logging controlled by a flag. Then update both
_mapAppStartedToState and _mapLoggedInToState methods to call this new private
method, removing duplicated code and replacing magic numbers with constants or
parameters.

} catch (_) {
emit(Unauthenticated());
}
}

_mapLoggedInToState(
AuthenticationEvent event, Emitter<AuthenticationState> emit) async {
final (userLoggedIn, user) = await _authRepository.getUser();

if (userLoggedIn && user != null) {
emit(Authenticated(user.toEntity()));
} else if (userLoggedIn && user == null) {
emit(OnboardingIncomplete());
} else {
emit(Unauthenticated());

int retries = 0;
const maxRetries = 3;
const retryDelay = Duration(milliseconds: 300);

while (retries < maxRetries) {
final (userLoggedIn, user) = await _authRepository.getUser();

if (userLoggedIn && user != null) {

emit(Authenticated(user.toEntity()));
return;
} else if (userLoggedIn && user == null) {
if (retries == maxRetries - 1) {

emit(OnboardingIncomplete());
return;
}
} else {

emit(Unauthenticated());
return;
}


retries++;
await Future.delayed(retryDelay);
}


emit(Unauthenticated());
}
Comment on lines +71 to 101
Copy link

@coderabbitai coderabbitai bot May 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply the same refactoring to avoid code duplication.

This method has identical retry logic to _mapAppStartedToState. Using the extracted _handleAuthenticationCheck method would eliminate this duplication.

 _mapLoggedInToState(
     AuthenticationEvent event, Emitter<AuthenticationState> emit) async {
-  // ... existing retry logic
+  await _handleAuthenticationCheck(emit);
 }

This also removes the need for try-catch here since the extracted method handles the authentication logic, and any unexpected errors would bubble up appropriately.

📝 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.

Suggested change
int retries = 0;
const maxRetries = 3;
const retryDelay = Duration(milliseconds: 300);
while (retries < maxRetries) {
final (userLoggedIn, user) = await _authRepository.getUser();
if (userLoggedIn && user != null) {
emit(Authenticated(user.toEntity()));
return;
} else if (userLoggedIn && user == null) {
if (retries == maxRetries - 1) {
emit(OnboardingIncomplete());
return;
}
} else {
emit(Unauthenticated());
return;
}
retries++;
await Future.delayed(retryDelay);
}
emit(Unauthenticated());
}
_mapLoggedInToState(
AuthenticationEvent event, Emitter<AuthenticationState> emit) async {
await _handleAuthenticationCheck(emit);
}
🤖 Prompt for AI Agents 
In lib/application/authentication/authentication_bloc.dart between lines 71 and
101, the retry logic in this method duplicates the logic already extracted into
the _handleAuthenticationCheck method used by _mapAppStartedToState. Refactor
this method to call _handleAuthenticationCheck instead of repeating the retry
loop and authentication checks. This will remove code duplication and also
eliminate the need for a try-catch block here since _handleAuthenticationCheck
manages errors and authentication logic centrally.


_mapLogOutToState(
Expand Down
15 changes: 12 additions & 3 deletions lib/application/authentication/login_register/login_register_bloc.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ class LoginRegisterBloc extends Bloc<LoginRegisterEvent, LoginRegisterState> {
emit(LoginRegisterLoading());

final map = await _authRepository.signInWithGoogle();

// Add delay before updating authentication state
await Future.delayed(const Duration(milliseconds: 200));

// Then update authentication state
_authenticationBloc.add(LoggedIn());
Comment on lines +102 to 107
Copy link

@coderabbitai coderabbitai bot May 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Reconsider the fixed delay approach for state synchronization.

The 200ms delay before updating authentication state is a brittle approach to handle data consistency. This could lead to issues in varying network conditions and doesn't guarantee that Firestore operations have completed.

Consider these more robust alternatives:

  1. Implement proper completion callbacks from the repository layer
  2. Use Firebase Auth state change listeners to trigger authentication updates
  3. Implement retry mechanisms in the data fetching layer instead of fixed delays
- // Add delay before updating authentication state
- await Future.delayed(const Duration(milliseconds: 200));
- 
- // Then update authentication state
+ // Update authentication state immediately - let AuthenticationBloc handle retries
 _authenticationBloc.add(LoggedIn());

The AuthenticationBloc already has retry logic, so the delays here might be redundant.

🤖 Prompt for AI Agents 
In lib/application/authentication/login_register/login_register_bloc.dart around
lines 102 to 107, replace the fixed 200ms delay before updating the
authentication state with a more reliable synchronization method. Remove the
Future.delayed call and instead ensure that the authentication state update
(_authenticationBloc.add(LoggedIn())) is triggered only after confirming that
Firestore operations have fully completed, either by using completion callbacks
from the repository, listening to Firebase Auth state changes, or relying on
existing retry logic in the AuthenticationBloc to handle consistency without
arbitrary delays.


if (map['isNewUser'] as bool) {
Expand All @@ -108,8 +113,6 @@ class LoginRegisterBloc extends Bloc<LoginRegisterEvent, LoginRegisterState> {
user: map['user'] as UserModel));
} else {
UserModel? user = map['user'] as UserModel;

_authenticationBloc.add(LoggedIn());
emit(SigninWithGoogleSuccess(isNewUser: false, user: user));
}
} catch (e) {
Expand Down Expand Up @@ -140,8 +143,14 @@ class LoginRegisterBloc extends Bloc<LoginRegisterEvent, LoginRegisterState> {
username: event.username,
profilePictureUrl: url);
if (user != null) {
emit(SignUpSuccess(user));
// First wait for a delay to ensure Firestore propagation
await Future.delayed(const Duration(milliseconds: 200));

// Then add the LoggedIn event
_authenticationBloc.add(LoggedIn());

// Then emit SignUpSuccess
emit(SignUpSuccess(user));
Comment on lines +146 to +153
Copy link

@coderabbitai coderabbitai bot May 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Same timing concern applies to the signup flow.

The fixed 200ms delay approach has the same reliability issues as mentioned in the Google signin flow. The comment mentions "Firestore propagation" but this doesn't guarantee the operation has completed.

Since the repository's signUp method should handle the atomicity of user creation, consider removing this delay and letting the AuthenticationBloc's retry mechanism handle any timing issues:

- // First wait for a delay to ensure Firestore propagation
- await Future.delayed(const Duration(milliseconds: 200));
- 
- // Then add the LoggedIn event
+ // Add the LoggedIn event - AuthenticationBloc will handle retries
  _authenticationBloc.add(LoggedIn());
- 
- // Then emit SignUpSuccess
  emit(SignUpSuccess(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.

Suggested change
// First wait for a delay to ensure Firestore propagation
await Future.delayed(const Duration(milliseconds: 200));
// Then add the LoggedIn event
_authenticationBloc.add(LoggedIn());
// Then emit SignUpSuccess
emit(SignUpSuccess(user));
// Add the LoggedIn event - AuthenticationBloc will handle retries
_authenticationBloc.add(LoggedIn());
emit(SignUpSuccess(user));
🤖 Prompt for AI Agents 
In lib/application/authentication/login_register/login_register_bloc.dart around
lines 146 to 153, remove the fixed 200ms delay that waits for Firestore
propagation before adding the LoggedIn event and emitting SignUpSuccess. This
delay is unreliable and unnecessary because the signUp method in the repository
should ensure atomic user creation. Instead, directly add the LoggedIn event and
emit SignUpSuccess, allowing the AuthenticationBloc's retry mechanism to handle
any timing or propagation issues.

} else {
emit(const SignUpFailed(message: 'Failed to sign up'));
}
Expand Down
28 changes: 27 additions & 1 deletion lib/data/models/user_model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ class UserModel {
final List<String> following;
final List<String> followers;
final List<String> posts;

final List<String> searchParams;

final List<MonumentModel> savedMonuments;
final List<String> myCommunities;
final List<String> joinedCommunities;
Expand All @@ -25,11 +28,11 @@ class UserModel {
final int monumentsSubmitted;
final int reviewsCompleted;
final List<String> votedMonuments;

const UserModel({
this.following = const [],
this.followers = const [],
this.posts = const [],
this.searchParams = const [],
required this.email,
required this.uid,
this.name = "Monumento User",
Expand Down Expand Up @@ -63,6 +66,9 @@ class UserModel {
followers: entity.followers,
following: entity.following,
posts: entity.posts,

searchParams: entity.searchParams,

savedMonuments: entity.savedMonuments
.map((monument) => MonumentModel.fromEntity(monument))
.toList(),
Expand All @@ -74,6 +80,7 @@ class UserModel {
monumentsSubmitted: entity.monumentsSubmitted,
reviewsCompleted: entity.reviewsCompleted,
votedMonuments: List<String>.from(entity.votedMonuments),

);
}

Expand All @@ -88,6 +95,9 @@ class UserModel {
followers: followers,
following: following,
posts: posts,

searchParams: searchParams,

myCommunities: List<String>.from(myCommunities),
joinedCommunities: List<String>.from(joinedCommunities),
// New mappings
Expand All @@ -98,6 +108,7 @@ class UserModel {
votedMonuments: List<String>.from(votedMonuments),
savedMonuments:
savedMonuments.map((monument) => monument.toEntity()).toList(),

);
}

Expand All @@ -111,6 +122,17 @@ class UserModel {
List<String> mappedPosts = data['posts'] != null
? (data['posts'] as List).map<String>((e) => e).toList()
: [];
List<String> mappedSearchParams = data['searchParams'] != null
? (data['searchParams'] as List).map<String>((e) => e).toList()
: [];

List<MonumentModel> mappedSavedMonuments = data['savedMonuments'] != null
? (data['savedMonuments'] as List)
.map((e) => MonumentModel.fromJson(e as Map<String, dynamic>))
.toList()
: [];



return UserModel(
uid: data['uid'] as String,
Expand All @@ -121,7 +143,9 @@ class UserModel {
username: data['username'] as String,
followers: mappedFollowers,
following: mappedFollowing,
savedMonuments: mappedSavedMonuments,
posts: mappedPosts,
searchParams: mappedSearchParams,
joinedCommunities: data['joinedCommunities'] as List<String>,
myCommunities: data['myCommunities'] as List<String>,
// New fields
Expand All @@ -145,6 +169,8 @@ class UserModel {
'followers': followers,
'following': following,
'posts': posts,
'savedMonuments': savedMonuments.map((m) => m.toJson()).toList(),
'searchParams': searchParams,
'myCommunities': myCommunities,
'joinedCommunities': joinedCommunities,
// New mappings
Expand Down
Loading