Fix Issue #369: Implement automatic login redirect after successful signup #371
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ccessful signup
WalkthroughThis PR adds retry logic to authentication user-fetching, inserts short delays and reorders dispatches in sign-in/sign-up flows, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginRegister as LoginRegisterBloc
participant AuthRepo as AuthRepository
participant AuthBloc as AuthenticationBloc
participant Router
User->>LoginRegister: complete sign-up / sign-in
LoginRegister->>AuthRepo: create/sign-in user (backend)
LoginRegister->>LoginRegister: await 200ms
LoginRegister->>AuthBloc: dispatch LoggedIn
LoginRegister-->>User: emit SignUpSuccess / SignedIn (after dispatch)
AuthBloc->>AuthRepo: getUser() <-- triggered by LoggedIn handling
AuthRepo->>AuthRepo: attempt 1
Note right of AuthRepo: if null -> wait 300ms and retry (up to 3)
AuthRepo-->>AuthBloc: user object or null
alt user present
AuthBloc->>AuthBloc: emit Authenticated
Router->>User: navigate Home
else user null after retries
AuthBloc->>AuthBloc: emit OnboardingIncomplete or Unauthenticated
Router->>User: navigate Onboarding or Login
end
sequenceDiagram
participant AppStart as App
participant AuthRepo as AuthRepository
participant AuthBloc as AuthenticationBloc
AppStart->>AuthBloc: AppStarted event
AuthBloc->>AuthRepo: getUser()
AuthRepo->>AuthRepo: attempt 1
Note over AuthRepo: if response null -> 300ms delay and retry
AuthRepo->>AuthRepo: attempt 2
AuthRepo-->>AuthBloc: user or null
alt user present
AuthBloc->>AuthBloc: emit Authenticated
else user null after retries
AuthBloc->>AuthBloc: emit Unauthenticated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
lib/router.dart (1)
17-36: Review the redirect logic and consider potential edge cases.The redirect logic handles the main authentication states correctly, but consider these potential improvements:
Service locator coupling: Directly accessing
locator<AuthenticationBloc>().statein the router creates tight coupling. Consider passing the authentication state through context or using a more dependency injection-friendly approach.Error handling: Add error handling for cases where the service locator might fail or the AuthenticationBloc is not properly initialized.
State exhaustiveness: Consider adding explicit handling for any other possible authentication states to make the logic more defensive.
redirect: (context, state) { + try { final authState = locator<AuthenticationBloc>().state; // Allow access if authenticated if (authState is Authenticated) { return null; } // Allow access if currently authenticating (during signup flow) if (authState is Uninitialized) { return null; } // Only redirect to login if definitely unauthenticated if (authState is Unauthenticated) { return '/login'; } return null; + } catch (e) { + // Fallback to login if authentication state cannot be determined + return '/login'; + } },lib/data/repositories/firebase_authentication_repository.dart (1)
247-254: Consider enhancing the exception class for better error handling.The current implementation is functional but could be improved to provide more structured error information.
Consider adding error codes or types for better error handling:
class AuthenticationFailure implements Exception { final String message; + final String? code; + final dynamic originalError; - AuthenticationFailure(this.message); + AuthenticationFailure(this.message, {this.code, this.originalError}); + + factory AuthenticationFailure.fromFirebaseException(FirebaseAuthException e) { + return AuthenticationFailure( + e.message ?? 'Authentication failed', + code: e.code, + originalError: e, + ); + } @override String toString() => message; }Then update usage:
- } on FirebaseAuthException catch (e) { - throw AuthenticationFailure(e.message ?? 'Authentication failed'); + } on FirebaseAuthException catch (e) { + throw AuthenticationFailure.fromFirebaseException(e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/application/authentication/authentication_bloc.dart(1 hunks)lib/application/authentication/login_register/login_register_bloc.dart(2 hunks)lib/data/models/user_model.dart(6 hunks)lib/data/repositories/firebase_authentication_repository.dart(3 hunks)lib/domain/entities/user_entity.dart(6 hunks)lib/main.dart(3 hunks)lib/router.dart(1 hunks)
🔇 Additional comments (3)
lib/domain/entities/user_entity.dart (1)
16-16: Well-implemented addition of the searchParams field.The
searchParamsfield has been consistently added across all parts of theUserEntityclass:
- ✅ Added to class definition with proper typing
- ✅ Included in constructor with sensible default (empty list)
- ✅ Handled in both
fromMapandfromSnapshotfactory methods with null safety- ✅ Included in
toMapserialization- ✅ Maintains backward compatibility
The implementation follows proper patterns and maintains consistency with existing list fields like
following,followers, andposts.Also applies to: 22-22, 46-48, 59-60, 74-76, 87-88, 105-105
lib/data/models/user_model.dart (1)
17-17: Well-implemented field addition!The
searchParamsfield has been consistently integrated throughout the model class, following the same patterns as existing fields. The null-safe handling infromMapand proper initialization with an empty list are good practices.Also applies to: 23-23, 49-49, 64-64, 78-80, 92-92, 107-107
lib/main.dart (1)
85-87: Verify UI scaling across different device sizes.The fixed design size of 390x844 (iPhone 12/13 dimensions) might not scale optimally on tablets or desktop screens that were previously handled with conditional logic.
Please ensure the UI looks correct on:
- Tablets (both portrait and landscape)
- Desktop/web views
- Smaller phones (e.g., iPhone SE)
The
splitScreenMode: truehelps, but visual testing across devices is recommended.
Comment on lines
+102
to
107
|
|
||
| // Add delay before updating authentication state | ||
| await Future.delayed(const Duration(milliseconds: 200)); | ||
|
|
||
| // Then update authentication state | ||
| _authenticationBloc.add(LoggedIn()); |
There was a problem hiding this comment.
🛠️ 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:
- Implement proper completion callbacks from the repository layer
- Use Firebase Auth state change listeners to trigger authentication updates
- 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.
Comment on lines
+146
to
+153
| // 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)); |
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+71
to
101
| 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()); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+32
to
+62
| 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()); |
There was a problem hiding this comment.
🛠️ 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:
- Code duplication: The retry logic is duplicated between
_mapAppStartedToStateand_mapLoggedInToState - Magic numbers: Retry count and delay should be configurable constants
- 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.
lib/main.dart
Outdated
Comment on lines
29
to
36
| try { | ||
| await Firebase.initializeApp( | ||
| options: DefaultFirebaseOptions.currentPlatform, | ||
| ); | ||
| debugPrint('Firebase initialized successfully'); | ||
| } catch (e) { | ||
| debugPrint('Firebase initialization failed: $e'); | ||
| } |
There was a problem hiding this comment.
Consider handling Firebase initialization failures more robustly.
Currently, the app continues running even if Firebase initialization fails. This could lead to runtime errors when Firebase services are accessed later in the app.
Consider either:
- Showing an error screen to the user
- Retrying initialization with exponential backoff
- At minimum, setting a flag to prevent Firebase service usage
try {
await Firebase.initializeApp(
options: DefaultFirebaseOptions.currentPlatform,
);
debugPrint('Firebase initialized successfully');
} catch (e) {
debugPrint('Firebase initialization failed: $e');
+ // Consider showing an error screen or retrying
+ // throw Exception('Failed to initialize Firebase: $e');
}📝 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
| try { | |
| await Firebase.initializeApp( | |
| options: DefaultFirebaseOptions.currentPlatform, | |
| ); | |
| debugPrint('Firebase initialized successfully'); | |
| } catch (e) { | |
| debugPrint('Firebase initialization failed: $e'); | |
| } | |
| try { | |
| await Firebase.initializeApp( | |
| options: DefaultFirebaseOptions.currentPlatform, | |
| ); | |
| debugPrint('Firebase initialized successfully'); | |
| } catch (e) { | |
| debugPrint('Firebase initialization failed: $e'); | |
| // Consider showing an error screen or retrying | |
| // throw Exception('Failed to initialize Firebase: $e'); | |
| } |
🤖 Prompt for AI Agents
In lib/main.dart around lines 29 to 36, the current Firebase initialization
catches errors but allows the app to continue running, which may cause runtime
issues later. Improve robustness by implementing one of the suggested
strategies: show an error screen if initialization fails, implement a retry
mechanism with exponential backoff, or at least set a flag indicating Firebase
is not initialized to prevent usage of Firebase services until successful
initialization.
Comment on lines
106
to
116
| final userData = { | ||
| 'uid': user.uid, | ||
| 'email': email, | ||
| 'name': name, | ||
| 'username': username, | ||
| 'status': status, | ||
| 'profilePictureUrl': profilePictureUrl, | ||
| 'searchParams': searchParams, | ||
| 'followers': [], | ||
| 'following': [], | ||
| }; |
There was a problem hiding this comment.
Add missing 'posts' field for consistency.
The userData map is missing the 'posts' field, which exists in UserModel. While the model handles this with a default value, it's better to be explicit for consistency with other methods.
final userData = {
'uid': user.uid,
'email': email,
'name': name,
'username': username,
'status': status,
'profilePictureUrl': profilePictureUrl,
'searchParams': searchParams,
'followers': [],
'following': [],
+ 'posts': [],
};📝 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
| final userData = { | |
| 'uid': user.uid, | |
| 'email': email, | |
| 'name': name, | |
| 'username': username, | |
| 'status': status, | |
| 'profilePictureUrl': profilePictureUrl, | |
| 'searchParams': searchParams, | |
| 'followers': [], | |
| 'following': [], | |
| }; | |
| final userData = { | |
| 'uid': user.uid, | |
| 'email': email, | |
| 'name': name, | |
| 'username': username, | |
| 'status': status, | |
| 'profilePictureUrl': profilePictureUrl, | |
| 'searchParams': searchParams, | |
| 'followers': [], | |
| 'following': [], | |
| 'posts': [], | |
| }; |
🤖 Prompt for AI Agents
In lib/data/repositories/firebase_authentication_repository.dart between lines
106 and 116, the userData map lacks the 'posts' field present in UserModel. Add
the 'posts' field explicitly to the userData map, initializing it appropriately
(e.g., as an empty list) to maintain consistency with other methods and the
model definition.
Comment on lines
118
to
122
| // 4. Force synchronous document creation | ||
| await _database.collection('users').doc(user.uid).set(userData); | ||
|
|
||
| // 5. Add a short delay to allow Firestore propagation | ||
| await Future.delayed(const Duration(milliseconds: 200)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more robust approach than fixed delay.
The 200ms delay is a workaround for Firestore's eventual consistency but may not be sufficient under poor network conditions or high load.
Consider one of these approaches:
- Use a transaction to ensure atomicity
- Verify document creation by reading it back
- Use Firestore's real-time listeners
Example of verification approach:
- // 4. Force synchronous document creation
- await _database.collection('users').doc(user.uid).set(userData);
-
- // 5. Add a short delay to allow Firestore propagation
- await Future.delayed(const Duration(milliseconds: 200));
+ // 4. Create document and verify
+ await _database.collection('users').doc(user.uid).set(userData);
+
+ // 5. Verify document exists with retry
+ DocumentSnapshot doc;
+ int retries = 0;
+ do {
+ doc = await _database.collection('users').doc(user.uid).get();
+ if (!doc.exists && retries < 3) {
+ await Future.delayed(const Duration(milliseconds: 100));
+ retries++;
+ }
+ } while (!doc.exists && retries < 3);
+
+ if (!doc.exists) {
+ throw AuthenticationFailure('Failed to create user document');
+ }📝 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
| // 4. Force synchronous document creation | |
| await _database.collection('users').doc(user.uid).set(userData); | |
| // 5. Add a short delay to allow Firestore propagation | |
| await Future.delayed(const Duration(milliseconds: 200)); | |
| // 4. Create document and verify | |
| await _database.collection('users').doc(user.uid).set(userData); | |
| // 5. Verify document exists with retry | |
| DocumentSnapshot doc; | |
| int retries = 0; | |
| do { | |
| doc = await _database.collection('users').doc(user.uid).get(); | |
| if (!doc.exists && retries < 3) { | |
| await Future.delayed(const Duration(milliseconds: 100)); | |
| retries++; | |
| } | |
| } while (!doc.exists && retries < 3); | |
| if (!doc.exists) { | |
| throw AuthenticationFailure('Failed to create user document'); | |
| } |
🤖 Prompt for AI Agents
In lib/data/repositories/firebase_authentication_repository.dart around lines
118 to 122, replace the fixed 200ms delay after setting the user document with a
more reliable method. Instead of awaiting a fixed delay, implement a
verification step by reading back the document after the set operation to
confirm its creation, or use a Firestore transaction to ensure atomicity, or
attach a real-time listener to detect when the document is available. This will
make the code more robust against network latency and Firestore's eventual
consistency.
Author
|
Thanks @
coderabbitai[bot]for the detailed review!
The core Issue #369 fix is working as intended. The suggestions around
delay optimization and code deduplication are excellent points for future
enhancement.
For now, the 200ms delays resolve the Firestore propagation timing issue
that was causing the original problem. I'll consider implementing the retry
verification approach in a follow-up PR.
The fixed delays are intentional and tested to work reliably for the signup
flow timing requirements.
…On Sat, 31 May 2025 at 17:56, coderabbitai[bot] ***@***.***> wrote:
***@***.***[bot]* commented on this pull request.
*Actionable comments posted: 7*
🧹 Nitpick comments (2)
lib/router.dart (1)
17-36: *Review the redirect logic and consider potential edge cases.*
The redirect logic handles the main authentication states correctly, but
consider these potential improvements:
1.
*Service locator coupling*: Directly accessing
locator<AuthenticationBloc>().state in the router creates tight
coupling. Consider passing the authentication state through context or
using a more dependency injection-friendly approach.
2.
*Error handling*: Add error handling for cases where the service
locator might fail or the AuthenticationBloc is not properly initialized.
3.
*State exhaustiveness*: Consider adding explicit handling for any
other possible authentication states to make the logic more defensive.
redirect: (context, state) {+ try {
final authState = locator<AuthenticationBloc>().state;
// Allow access if authenticated
if (authState is Authenticated) {
return null;
}
// Allow access if currently authenticating (during signup flow)
if (authState is Uninitialized) {
return null;
}
// Only redirect to login if definitely unauthenticated
if (authState is Unauthenticated) {
return '/login';
}
return null;+ } catch (e) {+ // Fallback to login if authentication state cannot be determined+ return '/login';+ }
},
lib/data/repositories/firebase_authentication_repository.dart (1)
247-254: *Consider enhancing the exception class for better error
handling.*
The current implementation is functional but could be improved to provide
more structured error information.
Consider adding error codes or types for better error handling:
class AuthenticationFailure implements Exception {
final String message;+ final String? code;+ final dynamic originalError;
- AuthenticationFailure(this.message);+ AuthenticationFailure(this.message, {this.code, this.originalError});+ + factory AuthenticationFailure.fromFirebaseException(FirebaseAuthException e) {+ return AuthenticationFailure(+ e.message ?? 'Authentication failed',+ code: e.code,+ originalError: e,+ );+ }
@OverRide
String toString() => message;
}
Then update usage:
- } on FirebaseAuthException catch (e) {- throw AuthenticationFailure(e.message ?? 'Authentication failed');+ } on FirebaseAuthException catch (e) {+ throw AuthenticationFailure.fromFirebaseException(e);
📜 Review details
*Configuration used: CodeRabbit UI*
*Review profile: CHILL*
*Plan: Pro*
📥 Commits
Reviewing files that changed from the base of the PR and between 7074a6e
<7074a6e>
and f3db9b7
<f3db9b7>
.
📒 Files selected for processing (7)
- lib/application/authentication/authentication_bloc.dart (1 hunks)
-
lib/application/authentication/login_register/login_register_bloc.dart
(2 hunks)
- lib/data/models/user_model.dart (6 hunks)
- lib/data/repositories/firebase_authentication_repository.dart (3
hunks)
- lib/domain/entities/user_entity.dart (6 hunks)
- lib/main.dart (3 hunks)
- lib/router.dart (1 hunks)
🔇 Additional comments (3)
lib/domain/entities/user_entity.dart (1)
16-16: *Well-implemented addition of the searchParams field.*
The searchParams field has been consistently added across all parts of
the UserEntity class:
- ✅ Added to class definition with proper typing
- ✅ Included in constructor with sensible default (empty list)
- ✅ Handled in both fromMap and fromSnapshot factory methods with null
safety
- ✅ Included in toMap serialization
- ✅ Maintains backward compatibility
The implementation follows proper patterns and maintains consistency with
existing list fields like following, followers, and posts.
Also applies to: 22-22, 46-48, 59-60, 74-76, 87-88, 105-105
lib/data/models/user_model.dart (1)
17-17: *Well-implemented field addition!*
The searchParams field has been consistently integrated throughout the
model class, following the same patterns as existing fields. The null-safe
handling in fromMap and proper initialization with an empty list are good
practices.
Also applies to: 23-23, 49-49, 64-64, 78-80, 92-92, 107-107
lib/main.dart (1)
85-87: *Verify UI scaling across different device sizes.*
The fixed design size of 390x844 (iPhone 12/13 dimensions) might not scale
optimally on tablets or desktop screens that were previously handled with
conditional logic.
Please ensure the UI looks correct on:
- Tablets (both portrait and landscape)
- Desktop/web views
- Smaller phones (e.g., iPhone SE)
The splitScreenMode: true helps, but visual testing across devices is
recommended.
------------------------------
In lib/application/authentication/login_register/login_register_bloc.dart
<#371 (comment)>:
> +
+ // Add delay before updating authentication state
+ await Future.delayed(const Duration(milliseconds: 200));
+
+ // Then update authentication state
_authenticationBloc.add(LoggedIn());
*🛠️ 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.
------------------------------
In lib/application/authentication/login_register/login_register_bloc.dart
<#371 (comment)>:
> + // 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));
*🛠️ 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
|
Author
|
Thank you @coderabbitai for the detailed review and suggestions! I
appreciate the points about retry logic, error handling, and code
structure. For this PR, my focus was on resolving the core authentication
issue for #369. I will consider these recommendations for future
enhancements or as requested by maintainers.
…On Sat, 31 May 2025 at 19:16, Vikas Naik ***@***.***> wrote:
Thanks @
coderabbitai[bot]for the detailed review!
The core Issue #369 fix is working as intended. The suggestions around
delay optimization and code deduplication are excellent points for future
enhancement.
For now, the 200ms delays resolve the Firestore propagation timing issue
that was causing the original problem. I'll consider implementing the retry
verification approach in a follow-up PR.
The fixed delays are intentional and tested to work reliably for the
signup flow timing requirements.
On Sat, 31 May 2025 at 17:56, coderabbitai[bot] ***@***.***>
wrote:
> ***@***.***[bot]* commented on this pull request.
>
> *Actionable comments posted: 7*
> 🧹 Nitpick comments (2)
>
> lib/router.dart (1)
>
> 17-36: *Review the redirect logic and consider potential edge cases.*
>
> The redirect logic handles the main authentication states correctly, but
> consider these potential improvements:
>
> 1.
>
> *Service locator coupling*: Directly accessing
> locator<AuthenticationBloc>().state in the router creates tight
> coupling. Consider passing the authentication state through context or
> using a more dependency injection-friendly approach.
> 2.
>
> *Error handling*: Add error handling for cases where the service
> locator might fail or the AuthenticationBloc is not properly initialized.
> 3.
>
> *State exhaustiveness*: Consider adding explicit handling for any
> other possible authentication states to make the logic more defensive.
>
> redirect: (context, state) {+ try {
> final authState = locator<AuthenticationBloc>().state;
>
> // Allow access if authenticated
> if (authState is Authenticated) {
> return null;
> }
>
> // Allow access if currently authenticating (during signup flow)
> if (authState is Uninitialized) {
> return null;
> }
>
> // Only redirect to login if definitely unauthenticated
> if (authState is Unauthenticated) {
> return '/login';
> }
>
> return null;+ } catch (e) {+ // Fallback to login if authentication state cannot be determined+ return '/login';+ }
> },
>
> lib/data/repositories/firebase_authentication_repository.dart (1)
>
> 247-254: *Consider enhancing the exception class for better error
> handling.*
>
> The current implementation is functional but could be improved to provide
> more structured error information.
>
> Consider adding error codes or types for better error handling:
>
> class AuthenticationFailure implements Exception {
> final String message;+ final String? code;+ final dynamic originalError;
> - AuthenticationFailure(this.message);+ AuthenticationFailure(this.message, {this.code, this.originalError});+ + factory AuthenticationFailure.fromFirebaseException(FirebaseAuthException e) {+ return AuthenticationFailure(+ e.message ?? 'Authentication failed',+ code: e.code,+ originalError: e,+ );+ }
>
> @OverRide
> String toString() => message;
> }
>
> Then update usage:
>
> - } on FirebaseAuthException catch (e) {- throw AuthenticationFailure(e.message ?? 'Authentication failed');+ } on FirebaseAuthException catch (e) {+ throw AuthenticationFailure.fromFirebaseException(e);
>
> 📜 Review details
>
> *Configuration used: CodeRabbit UI*
> *Review profile: CHILL*
> *Plan: Pro*
> 📥 Commits
>
> Reviewing files that changed from the base of the PR and between 7074a6e
> <7074a6e>
> and f3db9b7
> <f3db9b7>
> .
> 📒 Files selected for processing (7)
>
> - lib/application/authentication/authentication_bloc.dart (1 hunks)
> -
> lib/application/authentication/login_register/login_register_bloc.dart
> (2 hunks)
> - lib/data/models/user_model.dart (6 hunks)
> - lib/data/repositories/firebase_authentication_repository.dart (3
> hunks)
> - lib/domain/entities/user_entity.dart (6 hunks)
> - lib/main.dart (3 hunks)
> - lib/router.dart (1 hunks)
>
> 🔇 Additional comments (3)
>
> lib/domain/entities/user_entity.dart (1)
>
> 16-16: *Well-implemented addition of the searchParams field.*
>
> The searchParams field has been consistently added across all parts of
> the UserEntity class:
>
> - ✅ Added to class definition with proper typing
> - ✅ Included in constructor with sensible default (empty list)
> - ✅ Handled in both fromMap and fromSnapshot factory methods with
> null safety
> - ✅ Included in toMap serialization
> - ✅ Maintains backward compatibility
>
> The implementation follows proper patterns and maintains consistency with
> existing list fields like following, followers, and posts.
>
> Also applies to: 22-22, 46-48, 59-60, 74-76, 87-88, 105-105
> lib/data/models/user_model.dart (1)
>
> 17-17: *Well-implemented field addition!*
>
> The searchParams field has been consistently integrated throughout the
> model class, following the same patterns as existing fields. The null-safe
> handling in fromMap and proper initialization with an empty list are
> good practices.
>
> Also applies to: 23-23, 49-49, 64-64, 78-80, 92-92, 107-107
> lib/main.dart (1)
>
> 85-87: *Verify UI scaling across different device sizes.*
>
> The fixed design size of 390x844 (iPhone 12/13 dimensions) might not
> scale optimally on tablets or desktop screens that were previously handled
> with conditional logic.
>
> Please ensure the UI looks correct on:
>
> - Tablets (both portrait and landscape)
> - Desktop/web views
> - Smaller phones (e.g., iPhone SE)
>
> The splitScreenMode: true helps, but visual testing across devices is
> recommended.
>
> ------------------------------
>
> In lib/application/authentication/login_register/login_register_bloc.dart
> <#371 (comment)>
> :
>
> > +
> + // Add delay before updating authentication state
> + await Future.delayed(const Duration(milliseconds: 200));
> +
> + // Then update authentication state
> _authenticationBloc.add(LoggedIn());
>
> *🛠️ 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.
>
> ------------------------------
>
> In lib/application/authentication/login_register/login_register_bloc.dart
> <#371 (comment)>
> :
>
> > + // 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));
>
> *🛠️ 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
>
>
|
|
Tip For best results, initiate chat on the files or code changes. @vnaik10 You're absolutely right to prioritize fixing the core authentication issue first! 🎯 Your approach is sound - getting Issue #369 resolved with a working solution and then iterating on the architecture is good engineering practice. The 200ms delays may not be the most elegant solution, but if they're tested and working reliably for your use case, that's what matters for this PR. I appreciate you taking the time to consider the suggestions for future improvements. The retry verification approach and code deduplication can definitely wait for a follow-up PR when you have more bandwidth or when maintainers request those enhancements. Thanks for the thoughtful response, and great work getting the automatic login redirect working! The users will definitely appreciate not having to go through the extra login step after signup. Feel free to tag me if you need any assistance when you do tackle those architectural improvements in the future. 🚀 |
Author
|
All merge conflicts have been resolved. The PR is now aligned with the latest Appwrite migration changes. Ready for review again. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/application/authentication/login_register/login_register_bloc.dart (2)
101-107: Delay after Google sign‑in is acceptable as a tactical fix but should be revisited.The 200 ms sleep before dispatching
LoggedInis a pragmatic way to smooth out backend propagation, but it’s still a brittle heuristic and duplicates concerns already handled inAuthenticationBloc’s retry logic. Consider, in a follow‑up, moving this timing concern into the repository/AuthBloc (e.g., via completion signals or read‑back) and removing the hardcoded delay so login isn’t coupled to a magic number.
145-153: Signup delay + LoggedIn ordering works but comment and timing deserve cleanup later.The 200 ms delay before
LoggedInfollowed bySignUpSuccessmatches the intended “auto‑login after signup” behavior and appears to fix the race you were targeting. That said, it’s still a fixed‑delay workaround, and the inline “Firestore propagation” comment is now a bit misleading given the Appwrite‑backed repo. In a future pass, consider:
- Replacing the delay with a more deterministic sync (repo‑level confirmation / AuthBloc retry only).
- Updating the comment to describe generic backend propagation rather than Firestore specifically.
Not blocking this PR, but worth tracking as tech debt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/application/authentication/login_register/login_register_bloc.dart(2 hunks)lib/data/models/user_model.dart(9 hunks)lib/domain/entities/user_entity.dart(3 hunks)lib/main.dart(3 hunks)
🔇 Additional comments (5)
lib/main.dart (3)
9-13: Root AuthenticationBloc wiring aligns with the new auth‑driven routing.Providing
AuthenticationBlocat the top level viaMultiBlocProviderand the service locator matches the new automatic login / routing flow and should keep auth state globally accessible without extra wrapper widgets, assuming no other conflictingAuthenticationBlocproviders remain.Please double‑check that any previous
AuthenticationBlocproviders (e.g., inside removed Wrapper or other shells) have been cleaned up so you don’t end up with multiple bloc instances in the tree.Also applies to: 36-41
63-73: Centralized designSize logic looks consistent across platforms.The new
designSizebranching onkIsWebvs. (iOS/Android) vs. other platforms, with the width threshold at 530 px, is clear and should make responsive tuning easier going forward; no obvious edge cases stand out here.
91-94: Theme + ScreenUtil/breakpoints wiring remains coherent with the previous layout.Adding a
TabBarThemeand passing the computeddesignSizedirectly intoScreenUtilInit(while preserving the existing breakpoint definitions) keeps the responsive behavior intact and clarifies where typography/layout scaling is configured. Looks good; just ensure you’ve sanity‑checked on a small mobile width and a large desktop width.Also applies to: 100-108
lib/data/models/user_model.dart (1)
19-20: searchParams and savedMonuments mappings are consistent end‑to‑end.The new
searchParamsfield and its defaults, plus the added mappings infromEntity,toEntity,fromMap, andtoMapare all symmetric and safe (they fall back to empty lists when absent). The refactor forsavedMonumentsto go throughMonumentModel.fromJson/toJsonalso looks coherent with the rest of the model.Also applies to: 35-36, 69-71, 99-100, 125-127, 129-133, 146-148, 172-173
lib/domain/entities/user_entity.dart (1)
19-20: New searchParams and savedMonuments support is wired correctly across entity and map.The added
searchParamsfield (with an empty‑list default) and its inclusion inprops,fromDocument,fromMap, andtoMapall line up. Likewise, thesavedMonumentshandling viaMonumentEntity.fromMap(...)on read and.toMap()on write keeps the serialization round‑trip consistent. Defaults viaList<String>.from(... ?? [])and the null‑aware savedMonuments mapping should also protect against missing fields in older documents.Also applies to: 34-35, 52-71, 73-99, 106-131, 133-147
lib/domain/entities/user_entity.dart
Outdated
Comment on lines
102
to
104
| factory UserEntity.fromSnapshot(DocumentSnapshot snap) { | ||
| return UserEntity.fromMap(snap.data() as Map<String, dynamic>); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Undefined DocumentSnapshot in fromSnapshot will break compilation unless imported or aliased.
UserEntity.fromSnapshot(DocumentSnapshot snap) references DocumentSnapshot, but this file only imports appwrite/models.dart and equatable.dart; there’s no visible import for the Firestore DocumentSnapshot type (or any local alias). This will cause a compile error unless DocumentSnapshot is defined elsewhere in your project’s imports.
You likely need to either:
- Re‑add the appropriate Firestore import (e.g.,
import 'package:cloud_firestore/cloud_firestore.dart';) if this factory is still used, or - Remove/replace this factory entirely if your data layer has fully migrated to Appwrite
Documentand you no longer rely on Firestore snapshots.
Given this touches core user serialization, it should be fixed before merging.
Please verify with your analyzer/build that DocumentSnapshot resolves correctly in this file; if not, update the imports or remove this constructor.
I'll verify the review comment by examining the actual file and checking whether DocumentSnapshot is truly undefined.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check full contents of the file to see all imports
cat lib/domain/entities/user_entity.dart
Length of output: 5095
🏁 Script executed:
#!/bin/bash
# Search for DocumentSnapshot definition or import across the codebase
rg -n "DocumentSnapshot" --type dartLength of output: 1588
🏁 Script executed:
#!/bin/bash
# Check if fromSnapshot factory is used anywhere
rg -n "fromSnapshot" --type dart -C 2Length of output: 1090
Remove the undefined fromSnapshot factory method—it references undefined DocumentSnapshot and is dead code.
The review comment is correct. Verification confirms:
DocumentSnapshotis not imported inuser_entity.dart- The factory method at lines 102-104 references this undefined type, causing a compilation error
- The method is dead code: only callers are commented out in
firebase_social_repository.dart - Appwrite's
Documenttype is already the primary data layer (seefromDocumentfactory)
Remove lines 102-104 entirely since the codebase has migrated from Firebase to Appwrite and this factory is no longer needed.
🤖 Prompt for AI Agents
In lib/domain/entities/user_entity.dart around lines 102 to 104, remove the
entire factory UserEntity.fromSnapshot(DocumentSnapshot snap) { return
UserEntity.fromMap(snap.data() as Map<String, dynamic>); } since
DocumentSnapshot is undefined and the factory is dead code after migrating from
Firebase to Appwrite; delete these lines and ensure no leftover references
remain (no new imports or usages added).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/domain/entities/user_entity.dart (1)
19-19: searchParams field added consistently.The new field follows the same pattern as other list fields and is correctly integrated throughout the entity. Consider adding a documentation comment explaining its purpose and expected content for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/domain/entities/user_entity.dart(3 hunks)
🔇 Additional comments (3)
lib/domain/entities/user_entity.dart (3)
34-34: LGTM: Constructor parameter added correctly.The
searchParamsparameter uses the same default empty list pattern as other collection fields, maintaining consistency.
86-86: Serialization paths correctly implemented.The
searchParamsfield is parsed and serialized consistently acrossfromDocument,fromMap, andtoMapmethods, using null-safe defaults and appropriate type conversions.Also applies to: 113-113, 140-140
102-127: Past review comment appears resolved.The previous review flagged an undefined
fromSnapshotfactory method at lines 102-104. In the current code, these lines contain thefromMapfactory method, and nofromSnapshotmethod is present. The issue appears to have been addressed.
Author
|
Hi maintainers 👋 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🐛 Issue Fixed
Fixes #369 - Users were being redirected to login page after successful signup instead of being automatically logged in and taken to home screen.
✨ Solution Overview
Implemented comprehensive authentication flow improvements with robust multi-layer error handling to create a seamless user experience! 🚀
🛠️ Changes Made
🗄️ Repository Layer
✅ Added atomic document creation with 200ms propagation delay
✅ Enhanced data consistency for user creation
🧩 Bloc Layer
✅ Implemented 3-retry mechanism with 300ms intervals for authentication state
✅ Improved state management reliability
📊 Data Models
✅ Added searchParams field to UserModel/UserEntity
✅ Enhanced search functionality capabilities
🧭 Router Layer
✅ Added signup-specific navigation guards
✅ Prevents redirect loops and improves flow control
🔥 Firebase Setup
✅ Updated configuration for better performance
✅ Added firebase_options.dart to gitignore for open source compatibility
🔧 Technical Improvements
🎯 Enhanced BlocBuilder type safety across authentication components
🛡️ Improved error handling throughout signup flow
⚡ Resolved navigation timing issues during authentication state changes
🔄 Better state synchronization between Firebase Auth and Firestore
🧪 Testing Results
✅ End-to-end signup flow - Works perfectly!
✅ Automatic redirect to home screen - No more manual login required
✅ Zero crashes or navigation loops - Stable and reliable
✅ Firebase integration - Authentication + Firestore working seamlessly
🎉 Impact & Benefits
🚀 Improved UX: Users now automatically logged in after successful signup
🛡️ Enhanced Stability: Better authentication state management prevents edge cases
🔄 Backward Compatible: All existing functionality preserved
📱 Smoother Onboarding: Reduces friction in user registration process
Closes #369 🔒
issue.369.fixed.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Chores
UI
✏️ Tip: You can customize this high-level summary in your review settings.