Error handling #13
Error handling #13
Conversation
# Conflicts: # khelo/assets/locales/app_en.arb # khelo/lib/ui/flow/matches/match_list_screen.dart # khelo/lib/ui/flow/score_board/score_board_screen.dart
WalkthroughThis update enriches error handling throughout the application, introducing new error classes and localized error messages. It enhances error management in various services, improving user experience and ensuring smoother interactions. New UI components like Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files not reviewed due to errors (1)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (3)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
# Conflicts: # data/lib/service/ball_score/ball_score_service.dart # data/lib/service/innings/inning_service.dart # data/lib/service/match/match_service.dart # khelo/assets/locales/app_en.arb # khelo/lib/ui/flow/matches/match_detail/components/match_detail_commentary_view.dart # khelo/lib/ui/flow/matches/match_detail/components/match_detail_highlight_view.dart # khelo/lib/ui/flow/matches/match_detail/components/match_detail_info_view.dart # khelo/lib/ui/flow/matches/match_detail/components/match_detail_overs_view.dart # khelo/lib/ui/flow/matches/match_detail/components/match_detail_scorecard_view.dart # khelo/lib/ui/flow/matches/match_detail/components/match_detail_squad_view.dart # khelo/lib/ui/flow/matches/match_detail/match_detail_tab_view_model.dart
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Out of diff range and nitpick comments (16)
khelo/lib/components/error_screen.dart (1)
21-35: Consider replacing placeholder widgets with actual images in future updates.data/lib/service/file_upload/file_upload_service.dart (1)
25-27: Consider simplifying the image naming convention inuploadProfileImage.khelo/lib/components/place_holder_screen.dart (1)
63-68: Review the button's behavior whenonActionBtnTapis null.The
PrimaryButtonis rendered even ifonActionBtnTapis null, which might lead to a button that does nothing when tapped. Consider disabling the button or adjusting its visibility based on whetheronActionBtnTapis provided.khelo/lib/ui/flow/score_board/add_toss_detail/add_toss_detail_view_model.dart (2)
Line range hint
64-75: Review the state management during asynchronous operations.- state = state.copyWith(isTossDetailUpdateInProgress: true, actionError: null); + state = state.copyWith(isTossDetailUpdateInProgress: true);The method
onNextButtonTapclears theactionErrorbefore starting the operation, which might hide previous errors that are still relevant. Consider preserving the previous error until the operation is either completed or a new error is encountered.
86-86: Add nullability annotations to the error fields.Consider adding explicit nullability annotations to
errorandactionErrorfields inAddTossDetailStateto improve code clarity and enforce type safety.khelo/lib/ui/flow/sign_in/sign_in_with_phone/sign_in_with_phone_view_model.dart (1)
86-86: Clarify the purpose ofactionError.The
actionErrorfield inSignInWithPhoneStateis used to store errors from various operations. Consider adding documentation or comments explaining its usage and the types of errors it might contain, to aid in maintenance and further development.data/lib/service/ball_score/ball_score_service.dart (1)
27-43: Ensure atomicity in database operations.The method
updateBallScoreuses a batch to perform potentially multiple write operations. Ensure that these operations are atomic and consider handling potential partial updates that could lead to data inconsistency. Review the transaction management strategy to ensure data integrity.khelo/lib/ui/flow/sign_in/phone_verification/phone_verification_view_model.dart (3)
Line range hint
65-81: Enhance error handling in OTP resend.The method
resendCodehandles errors by settingactionError, but does not provide any user feedback or recovery options. Consider implementing a more comprehensive error handling strategy that includes user notifications and potential recovery actions.
Line range hint
89-104: Review error handling in OTP verification.The error handling in
verifyOTPdifferentiates between an invalid verification code and other errors, but does not provide clear recovery options for the user. Consider adding specific actions that the user can take in case of different types of errors, such as retrying the verification or contacting support.
119-119: Clarify the role ofactionErrorin the state.The
actionErrorfield inPhoneVerificationStateis used to store errors from various operations. Consider adding documentation or comments explaining its usage and the types of errors it might contain, to aid in maintenance and further development.data/lib/service/innings/inning_service.dart (1)
21-37: Ensure atomicity in database operations.The method
updateInninguses a batch to perform potentially multiple write operations. Ensure that these operations are atomic and consider handling potential partial updates that could lead to data inconsistency. Review the transaction management strategy to ensure data integrity.khelo/lib/ui/flow/matches/match_detail/components/match_detail_highlight_view.dart (2)
Line range hint
68-112: Consider using a more efficient method for sorting the highlights to improve performance.
Line range hint
112-142: The implementation of the highlight list is well-structured. Consider optimizing the _getHighlightsScore method for better performance.khelo/lib/ui/flow/score_board/add_toss_detail/add_toss_detail_screen.dart (1)
Line range hint
66-112: Consider using more descriptive variable names for better code readability in the _body method.khelo/lib/ui/flow/team/add_team/add_team_view_model.dart (2)
Line range hint
64-75: Handle potential null values in error handling.- state = state.copyWith(isImageUploading: false, actionError: e); + state = state.copyWith(isImageUploading: false, actionError: e.toString());When copying the error
etoactionError, ensure thateis converted to a string to avoid storing an exception object directly in the state, which could lead to serialization issues or unintended behavior in the UI.
Line range hint
212-238: Simplify control flow for image deletion.- if (teamProfileImageUrl != null) { - await deleteUnusedImage(teamProfileImageUrl); - } - if (currentImageUrl != null) { - await deleteUnusedImage(currentImageUrl); - } + List<String> imagesToDelete = []; + if (teamProfileImageUrl != null) { + imagesToDelete.add(teamProfileImageUrl); + } + if (currentImageUrl != null && currentImageUrl != teamProfileImageUrl) { + imagesToDelete.add(currentImageUrl); + } + for (var imgUrl in imagesToDelete) { + await deleteUnusedImage(imgUrl); + }Refactor the image deletion logic to collect all images to be deleted into a list and then delete them in a loop. This approach reduces code duplication and handles the case where both
teamProfileImageUrlandcurrentImageUrlmight be non-null and different more cleanly.
| } | ||
|
|
||
| Future<void> _getMatchById() async { | ||
| Future<void> getMatchById() async { |
There was a problem hiding this comment.
Consider handling potential null matchId more explicitly.
- if (matchId == null) {
+ if (matchId == null || matchId.isEmpty) {This change ensures that both null and empty strings are considered, preventing unnecessary API calls.
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.
| Future<void> getMatchById() async { | |
| Future<void> getMatchById() async { | |
| if (matchId == null || matchId.isEmpty) { |
| } | ||
|
|
||
| state = state.copyWith( | ||
| ballCount: ballCount, | ||
| totalRuns: state.totalRuns + | ||
| run + | ||
| (ball.inning_id != state.currentInning?.id && | ||
| extrasType == ExtrasType.penaltyRun | ||
| ? 0 | ||
| : extra ?? 0)); | ||
|
|
||
| if (wicketType != null) { | ||
| await _handleWicketAndOutPlayer(wicketType, playerOutId); | ||
| } | ||
| final nonStrikerId = state.batsMans | ||
| ?.firstWhere((element) => element.player.id != state.strikerId) | ||
| .player | ||
| .id ?? | ||
| "INVALID ID"; | ||
|
|
||
| final ball = BallScoreModel( | ||
| inning_id: inningId ?? state.currentInning?.id ?? "INVALID ID", | ||
| over_number: state.overCount, | ||
| ball_number: ballCount, | ||
| bowler_id: state.bowler?.player.id ?? "INVALID ID", | ||
| batsman_id: state.strikerId ?? "INVALID ID", | ||
| non_striker_id: nonStrikerId, | ||
| is_four: isFour, | ||
| is_six: isSix, | ||
| extras_awarded: extra, | ||
| extras_type: extrasType, | ||
| player_out_id: playerOutId, | ||
| runs_scored: run, | ||
| wicket_taker_id: wicketTakerId, | ||
| wicket_type: wicketType, | ||
| time: DateTime.now()); | ||
|
|
||
| String id = await _ballScoreService.updateBallScore(ball); | ||
|
|
||
| if (ball.inning_id != state.currentInning?.id && | ||
| extrasType == ExtrasType.penaltyRun) { | ||
| state = state.copyWith( | ||
| previousScoresList: [ | ||
| ...state.previousScoresList, | ||
| ball.copyWith(id: id) | ||
| ], | ||
| otherInning: state.otherInning?.copyWith( | ||
| total_runs: (state.otherInning?.total_runs ?? 0) + | ||
| (ball.extras_awarded ?? 0))); | ||
| } else { | ||
| state = state.copyWith(currentScoresList: [ | ||
| ...state.currentScoresList, | ||
| ball.copyWith(id: id) | ||
| ]); | ||
| } | ||
| await _updateInningAndTeamScore(); | ||
| ballCount: ballCount, | ||
| totalRuns: state.totalRuns + | ||
| run + | ||
| (ball.inning_id != state.currentInning?.id && | ||
| extrasType == ExtrasType.penaltyRun | ||
| ? 0 | ||
| : extra ?? 0)); | ||
|
|
||
| if (wicketType != null) { | ||
| await _handleWicketAndOutPlayer(wicketType, playerOutId); | ||
| } | ||
|
|
||
| if (switchStrike) { | ||
| setOrSwitchStriker(); | ||
| } | ||
| if (ball.inning_id != state.currentInning?.id && | ||
| extrasType == ExtrasType.penaltyRun) { | ||
| state = state.copyWith( | ||
| previousScoresList: [ | ||
| ...state.previousScoresList, | ||
| ball.copyWith(id: id) | ||
| ], | ||
| otherInning: state.otherInning?.copyWith( | ||
| total_runs: (state.otherInning?.total_runs ?? 0) + | ||
| (ball.extras_awarded ?? 0))); | ||
| } else { | ||
| state = state.copyWith(currentScoresList: [ | ||
| ...state.currentScoresList, | ||
| ball.copyWith(id: id) | ||
| ]); | ||
| } | ||
| await _updateInningAndTeamScore(); | ||
|
|
||
| if (switchStrike) { | ||
| setOrSwitchStriker(); | ||
| } | ||
|
|
||
| _checkIfInningComplete(wicket: wicketType); | ||
| _checkIfInningComplete(wicket: wicketType); | ||
| } catch (e, stack) { | ||
| state = state.copyWith(actionError: e); | ||
| debugPrint( | ||
| "ScoreBoardViewNotifier: error while adding ball -> $e, stack -> $stack"); | ||
| } |
There was a problem hiding this comment.
Complex method addBall needs refactoring for better readability.
Consider breaking down addBall into smaller, more manageable functions. This can improve readability and maintainability.
| } catch (error) { | ||
| throw AppError.fromError(error); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error);
+ throw AppError.fromError('Error fetching matches: ${error.toString()}');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.
| } catch (error) { | |
| throw AppError.fromError(error); | |
| } | |
| } catch (error) { | |
| throw AppError.fromError('Error fetching matches: ${error.toString()}'); | |
| } |
| } catch (error) { | ||
| throw AppError.fromError(error); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error);
+ throw AppError.fromError('Error fetching current user played matches: ${error.toString()}');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.
| } catch (error) { | |
| throw AppError.fromError(error); | |
| } | |
| } catch (error) { | |
| throw AppError.fromError('Error fetching current user played matches: ${error.toString()}'); | |
| } |
| try { | ||
| DocumentReference teamRef = | ||
| _firestore.collection(_collectionName).doc(team.id); | ||
| WriteBatch batch = _firestore.batch(); | ||
|
|
||
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | ||
| String newTeamId = teamRef.id; | ||
|
|
||
| if (team.id == null) { | ||
| batch.update(teamRef, {'id': newTeamId}); | ||
| } | ||
| await batch.commit(); | ||
| return newTeamId; | ||
| } catch (error) { | ||
| throw AppError.fromError(error); |
There was a problem hiding this comment.
Ensure robust error handling in Firestore operations.
- throw AppError.fromError(error);
+ if (error is FirebaseException) {
+ throw AppError.fromFirebaseException(error);
+ } else {
+ throw AppError.unknown();
+ }In the updateTeam method, when catching exceptions, differentiate between FirebaseException and other types of exceptions to provide more specific error handling. This change helps in debugging and provides more accurate feedback to the 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.
| try { | |
| DocumentReference teamRef = | |
| _firestore.collection(_collectionName).doc(team.id); | |
| WriteBatch batch = _firestore.batch(); | |
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | |
| String newTeamId = teamRef.id; | |
| if (team.id == null) { | |
| batch.update(teamRef, {'id': newTeamId}); | |
| } | |
| await batch.commit(); | |
| return newTeamId; | |
| } catch (error) { | |
| throw AppError.fromError(error); | |
| try { | |
| DocumentReference teamRef = | |
| _firestore.collection(_collectionName).doc(team.id); | |
| WriteBatch batch = _firestore.batch(); | |
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | |
| String newTeamId = teamRef.id; | |
| if (team.id == null) { | |
| batch.update(teamRef, {'id': newTeamId}); | |
| } | |
| await batch.commit(); | |
| return newTeamId; | |
| } catch (error) { | |
| if (error is FirebaseException) { | |
| throw AppError.fromFirebaseException(error); | |
| } else { | |
| throw AppError.unknown(); | |
| } |
| try { | ||
| await _firestore | ||
| .collection(_collectionName) | ||
| .doc(_currentUser?.id) | ||
| .delete(); | ||
| _currentUserJsonController.state = null; | ||
| } catch (error) { | ||
| throw AppError.fromError(error); | ||
| } |
There was a problem hiding this comment.
Handle errors in user deletion more gracefully.
The method deleteUser throws an AppError when an error occurs. Consider implementing a strategy to handle these errors more gracefully, such as retrying the operation or providing detailed feedback to the user about what went wrong.
| try { | ||
| DocumentReference inningRef = | ||
| _firestore.collection(_collectionName).doc(id); | ||
| DocumentSnapshot snapshot = await inningRef.get(); | ||
| Map<String, dynamic> inningData = snapshot.data() as Map<String, dynamic>; | ||
| var inningModel = InningModel.fromJson(inningData); | ||
| return inningModel; | ||
| } catch (error) { | ||
| throw AppError.fromError(error); | ||
| } |
There was a problem hiding this comment.
Improve error handling granularity.
The error handling in getInningById catches all exceptions and rethrows them as AppError. Consider distinguishing between different types of exceptions (e.g., network issues, permission errors) to provide more specific error messages and recovery options.
| controller.addError(AppError.fromError(error)); | ||
| } | ||
| }, onError: (error) { | ||
| controller.addError(error); | ||
| controller.addError(AppError.fromError(error)); |
There was a problem hiding this comment.
Handle stream errors more effectively.
The method getInningsStreamByMatchId adds errors to the stream but does not provide a mechanism to recover or retry. Consider implementing a retry strategy or providing a way for the client to request a reconnection.
| HomeViewNotifier notifier, | ||
| HomeViewState state, | ||
| ) { | ||
| if (state.loading) { | ||
| return const Center(child: AppProgressIndicator()); | ||
| } | ||
| if (state.error != null) { | ||
| return ErrorScreen( | ||
| error: state.error, | ||
| onRetryTap: notifier.loadMatches, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider using ListView.builder for the matches list to improve performance with large datasets.
| if (state.error != null) { | ||
| return ErrorScreen( | ||
| error: state.error, | ||
| onRetryTap: notifier.onSearchChanged, | ||
| ); | ||
| } | ||
|
|
||
| return state.searchedUsers.isEmpty | ||
| ? Center( | ||
| child: Text( | ||
| context.l10n.search_user_empty_text, | ||
| textAlign: TextAlign.center, | ||
| style: AppTextStyle.subtitle1.copyWith( | ||
| color: context.colorScheme.textDisabled, | ||
| fontSize: 20, | ||
| ), | ||
| ), | ||
| ) | ||
| : ListView.separated( | ||
| separatorBuilder: (context, index) { | ||
| return const SizedBox(height: 16); | ||
| }, | ||
| itemCount: state.searchedUsers.length, | ||
| itemBuilder: (context, index) { | ||
| UserModel user = state.searchedUsers[index]; | ||
| return _userProfileCell(context, notifier, state, user); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Consider using ListView.builder for the searched users list to improve performance with large datasets.
There was a problem hiding this comment.
Actionable comments posted: 21
Out of diff range and nitpick comments (1)
khelo/lib/ui/flow/sign_in/phone_verification/components/resend_code_view.dart (1)
11-16: Ensure consistent parameter naming.The parameters
countryCodeandphoneNumberare well-named, following the camelCase convention. Good job maintaining consistency in naming conventions.
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error, stack);
+ throw AppError.fromError('Error fetching matches: ${error.toString()}', stack);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.
| throw AppError.fromError(error, stack); | |
| } | |
| throw AppError.fromError('Error fetching matches: ${error.toString()}', stack); | |
| } |
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error, stack);
+ throw AppError.fromError('Error fetching current user played matches: ${error.toString()}', stack);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.
| throw AppError.fromError(error, stack); | |
| } | |
| throw AppError.fromError('Error fetching current user played matches: ${error.toString()}', stack); | |
| } |
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error, stack);
+ throw AppError.fromError('Error fetching current user related matches: ${error.toString()}', stack);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.
| throw AppError.fromError(error, stack); | |
| } | |
| throw AppError.fromError('Error fetching current user related matches: ${error.toString()}', stack); | |
| } |
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error, stack);
+ throw AppError.fromError('Error fetching matches by team ID: ${error.toString()}', stack);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.
| throw AppError.fromError(error, stack); | |
| } | |
| throw AppError.fromError('Error fetching matches by team ID: ${error.toString()}', stack); | |
| } |
| controller.addError(AppError.fromError(error, stack)); | ||
| } | ||
| }, onError: (error) { | ||
| controller.addError(error); | ||
| }, onError: (error, stack) { | ||
| controller.addError(AppError.fromError(error, stack)); |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- controller.addError(AppError.fromError(error, stack));
+ controller.addError(AppError.fromError('Error fetching running matches: ${error.toString()}', stack));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.
| controller.addError(AppError.fromError(error, stack)); | |
| } | |
| }, onError: (error) { | |
| controller.addError(error); | |
| }, onError: (error, stack) { | |
| controller.addError(AppError.fromError(error, stack)); | |
| controller.addError(AppError.fromError('Error fetching running matches: ${error.toString()}', stack)); | |
| } | |
| }, onError: (error, stack) { | |
| controller.addError(AppError.fromError('Error fetching running matches: ${error.toString()}', stack)); |
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Consider enhancing error messages for better debugging.
- throw AppError.fromError(error, stack);
+ throw AppError.fromError('Error fetching player list from player IDs: ${error.toString()}', stack);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.
| throw AppError.fromError(error, stack); | |
| } | |
| throw AppError.fromError('Error fetching player list from player IDs: ${error.toString()}', stack); | |
| } |
| final splitString = this!.split(" "); | ||
| if (splitString.length == 1) { | ||
| if (this!.length < 13) { | ||
| return _missing; | ||
| } | ||
| return context.l10n.common_obscure_phone_number_text( | ||
| this!.substring(0, 3), this!.substring(this!.length - 2)); | ||
| } else { | ||
| final countryCode = splitString.first; | ||
| final lastTwoDigit = splitString | ||
| .elementAt(1) | ||
| .substring(splitString.elementAt(1).length - 2); | ||
| return context.l10n | ||
| .common_obscure_phone_number_text(countryCode, lastTwoDigit); |
There was a problem hiding this comment.
Ensure proper handling of phone numbers with more than two segments.
The current implementation assumes phone numbers are either a single string or two segments separated by spaces. Consider cases where phone numbers might have more than two segments, which could lead to incorrect formatting or runtime errors.
| } catch (e) { | ||
| state = state.copyWith(loading: false, error: e); | ||
| debugPrint( | ||
| "MatchListViewNotifier: error while load matches -> $e ,\nstack: $stack"); | ||
| "MatchListViewNotifier: error while load matches -> $e"); |
There was a problem hiding this comment.
Improve error logging by including more context.
Consider enhancing the debug print statement to include more context about the error, such as the function where it occurred and possibly some input parameters if applicable. This will aid in debugging and maintaining the code.
- debugPrint("MatchListViewNotifier: error while load matches -> $e");
+ debugPrint("MatchListViewNotifier.loadMatches: Error occurred with details -> $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.
| } catch (e) { | |
| state = state.copyWith(loading: false, error: e); | |
| debugPrint( | |
| "MatchListViewNotifier: error while load matches -> $e ,\nstack: $stack"); | |
| "MatchListViewNotifier: error while load matches -> $e"); | |
| } catch (e) { | |
| state = state.copyWith(loading: false, error: e); | |
| debugPrint( | |
| "MatchListViewNotifier.loadMatches: Error occurred with details -> $e"); |
| try { | ||
| await _auth.verifyPhoneNumber( | ||
| phoneNumber: countryCode + phoneNumber, | ||
| verificationCompleted: (phoneAuthCredential) async { | ||
| final userCredential = | ||
| await _auth.signInWithCredential(phoneAuthCredential); | ||
| _onVerificationSuccess(countryCode, phoneNumber, userCredential); | ||
| onVerificationCompleted != null | ||
| ? onVerificationCompleted(phoneAuthCredential, userCredential) | ||
| : null; | ||
| }, | ||
| verificationFailed: (FirebaseAuthException e) => | ||
| onVerificationFailed != null | ||
| ? onVerificationFailed(AppError.fromError(e, e.stackTrace)) | ||
| : null, | ||
| codeSent: (String verificationId, int? resendToken) => | ||
| onCodeSent != null ? onCodeSent(verificationId, resendToken) : null, | ||
| codeAutoRetrievalTimeout: (verificationId) => | ||
| onCodeAutoRetrievalTimeout != null | ||
| ? onCodeAutoRetrievalTimeout(verificationId) | ||
| : null, | ||
| ); | ||
| } catch (error, stack) { | ||
| throw AppError.fromError(error, stack); | ||
| } |
There was a problem hiding this comment.
Ensure comprehensive error handling in verifyPhoneNumber.
The method verifyPhoneNumber uses a try-catch block to handle errors, which is good. However, ensure that all potential exceptions, especially those specific to phone number verification, are adequately handled and logged.
| try { | ||
| DocumentReference teamRef = | ||
| _firestore.collection(_collectionName).doc(team.id); | ||
| WriteBatch batch = _firestore.batch(); | ||
|
|
||
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | ||
| String newTeamId = teamRef.id; | ||
|
|
||
| if (team.id == null) { | ||
| batch.update(teamRef, {'id': newTeamId}); | ||
| } | ||
| await batch.commit(); | ||
| return newTeamId; | ||
| } catch (error, stack) { | ||
| throw AppError.fromError(error, stack); |
There was a problem hiding this comment.
Refine error handling in updateTeam.
In the updateTeam method, refine the error handling to differentiate between different types of Firestore exceptions. This will provide more specific feedback to the user and help in debugging.
- throw AppError.fromError(error, stack);
+ if (error is FirebaseException) {
+ throw AppError.fromFirebaseException(error);
+ } else {
+ throw AppError.unknown();
+ }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.
| try { | |
| DocumentReference teamRef = | |
| _firestore.collection(_collectionName).doc(team.id); | |
| WriteBatch batch = _firestore.batch(); | |
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | |
| String newTeamId = teamRef.id; | |
| if (team.id == null) { | |
| batch.update(teamRef, {'id': newTeamId}); | |
| } | |
| await batch.commit(); | |
| return newTeamId; | |
| } catch (error, stack) { | |
| throw AppError.fromError(error, stack); | |
| try { | |
| DocumentReference teamRef = | |
| _firestore.collection(_collectionName).doc(team.id); | |
| WriteBatch batch = _firestore.batch(); | |
| batch.set(teamRef, team.toJson(), SetOptions(merge: true)); | |
| String newTeamId = teamRef.id; | |
| if (team.id == null) { | |
| batch.update(teamRef, {'id': newTeamId}); | |
| } | |
| await batch.commit(); | |
| return newTeamId; | |
| } catch (error, stack) { | |
| if (error is FirebaseException) { | |
| throw AppError.fromFirebaseException(error); | |
| } else { | |
| throw AppError.unknown(); | |
| } |
cp-sidhdhi-p
force-pushed
the
sidhdhi/refactor-string-error-handling
branch
from
099a47a
to
2b1b882
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
khelo/lib/ui/flow/sign_in/phone_verification/phone_verification_view_model.dart (1)
2-3: Ensure that error handling classes are consistently used across the application for better maintainability.Consider using a centralized error handling mechanism or service to manage errors more effectively across different parts of the application.
| required String code, | ||
| }) { | ||
| state = state.copyWith(verificationId: verificationId); | ||
| phoneNumber = phone; | ||
| countryCode = code; |
There was a problem hiding this comment.
Properly document the method updateVerificationIdAndPhone to explain the use of countryCode.
+ /// Updates the verification ID and phone number along with the country code.
+ /// This method is typically called to reset these values during the verification process.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.
| required String code, | |
| }) { | |
| state = state.copyWith(verificationId: verificationId); | |
| phoneNumber = phone; | |
| countryCode = code; | |
| /// Updates the verification ID and phone number along with the country code. | |
| /// This method is typically called to reset these values during the verification process. | |
| required String code, | |
| }) { | |
| state = state.copyWith(verificationId: verificationId); | |
| phoneNumber = phone; | |
| countryCode = code; |
| Future<void> resendCode({required String countryCode, required String phone}) async { | ||
| try { | ||
| state = state.copyWith(showErrorVerificationCodeText: false); | ||
| state = state.copyWith(showErrorVerificationCodeText: false, actionError: null); | ||
| updateResendCodeTimerDuration(); | ||
| _authService.verifyPhoneNumber( | ||
| countryCode: countryCode, | ||
| phoneNumber: phone, | ||
| onVerificationCompleted: (phoneCredential, _) { | ||
| state = | ||
| state.copyWith(verifying: false, isVerificationComplete: true); | ||
| }, | ||
| onVerificationFailed: (error) { | ||
| state = state.copyWith(error: error); | ||
| state = state.copyWith(actionError: error); | ||
| }, | ||
| onCodeSent: (verificationId, _) { | ||
| state = state.copyWith(verificationId: verificationId); | ||
| }, | ||
| ); | ||
| } catch (e) { | ||
| state = state.copyWith(actionError: e); |
There was a problem hiding this comment.
Refactor resendCode to handle potential null values in parameters to prevent runtime exceptions.
- Future<void> resendCode({required String countryCode, required String phone}) async {
+ Future<void> resendCode({required String? countryCode, required String? phone}) async {
+ if (countryCode == null || phone == null) return;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.
| Future<void> resendCode({required String countryCode, required String phone}) async { | |
| try { | |
| state = state.copyWith(showErrorVerificationCodeText: false); | |
| state = state.copyWith(showErrorVerificationCodeText: false, actionError: null); | |
| updateResendCodeTimerDuration(); | |
| _authService.verifyPhoneNumber( | |
| countryCode: countryCode, | |
| phoneNumber: phone, | |
| onVerificationCompleted: (phoneCredential, _) { | |
| state = | |
| state.copyWith(verifying: false, isVerificationComplete: true); | |
| }, | |
| onVerificationFailed: (error) { | |
| state = state.copyWith(error: error); | |
| state = state.copyWith(actionError: error); | |
| }, | |
| onCodeSent: (verificationId, _) { | |
| state = state.copyWith(verificationId: verificationId); | |
| }, | |
| ); | |
| } catch (e) { | |
| state = state.copyWith(actionError: e); | |
| Future<void> resendCode({required String? countryCode, required String? phone}) async { | |
| if (countryCode == null || phone == null) return; | |
| try { | |
| state = state.copyWith(showErrorVerificationCodeText: false, actionError: null); | |
| updateResendCodeTimerDuration(); | |
| _authService.verifyPhoneNumber( | |
| countryCode: countryCode, | |
| phoneNumber: phone, | |
| onVerificationCompleted: (phoneCredential, _) { | |
| state = | |
| state.copyWith(verifying: false, isVerificationComplete: true); | |
| }, | |
| onVerificationFailed: (error) { | |
| state = state.copyWith(actionError: error); | |
| }, | |
| onCodeSent: (verificationId, _) { | |
| state = state.copyWith(verificationId: verificationId); | |
| }, | |
| ); | |
| } catch (e) { | |
| state = state.copyWith(actionError: e); |
| state.copyWith(verifying: true, showErrorVerificationCodeText: false, actionError: null); | ||
| try { | ||
| await _authService.verifyOTP(state.verificationId!, state.otp); | ||
| await _authService.verifyOTP(countryCode, phoneNumber, state.verificationId!, state.otp); | ||
| state = state.copyWith(verifying: false, isVerificationComplete: true); | ||
| } on FirebaseAuthException catch (e) { | ||
| if (e.code == "invalid-verification-code") { | ||
| } on AppError catch (e) { | ||
| if (e.l10nCode == AppErrorL10nCodes.invalidVerificationCode) { | ||
| state = state.copyWith( | ||
| verifying: false, showErrorVerificationCodeText: true); | ||
| } else { | ||
| //network-request-failed | ||
| state = state.copyWith(verifying: false, error: e); | ||
| state = state.copyWith(verifying: false, actionError: e); | ||
| } | ||
| debugPrint( | ||
| "PhoneVerificationViewNotifier: error in FirebaseAuthException: verifyOTP -> $e"); | ||
| "PhoneVerificationViewNotifier: error in AppError: verifyOTP -> $e"); | ||
| } catch (e) { | ||
| state = state.copyWith(verifying: false, error: e); | ||
| state = state.copyWith(verifying: false, actionError: e); |
There was a problem hiding this comment.
Optimize error handling in verifyOTP by consolidating error logging.
- debugPrint("PhoneVerificationViewNotifier: error in AppError: verifyOTP -> $e");
- debugPrint("PhoneVerificationViewNotifier: error in verifyOTP -> $e");
+ debugPrint("PhoneVerificationViewNotifier: error in verifyOTP -> $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.
| state.copyWith(verifying: true, showErrorVerificationCodeText: false, actionError: null); | |
| try { | |
| await _authService.verifyOTP(state.verificationId!, state.otp); | |
| await _authService.verifyOTP(countryCode, phoneNumber, state.verificationId!, state.otp); | |
| state = state.copyWith(verifying: false, isVerificationComplete: true); | |
| } on FirebaseAuthException catch (e) { | |
| if (e.code == "invalid-verification-code") { | |
| } on AppError catch (e) { | |
| if (e.l10nCode == AppErrorL10nCodes.invalidVerificationCode) { | |
| state = state.copyWith( | |
| verifying: false, showErrorVerificationCodeText: true); | |
| } else { | |
| //network-request-failed | |
| state = state.copyWith(verifying: false, error: e); | |
| state = state.copyWith(verifying: false, actionError: e); | |
| } | |
| debugPrint( | |
| "PhoneVerificationViewNotifier: error in FirebaseAuthException: verifyOTP -> $e"); | |
| "PhoneVerificationViewNotifier: error in AppError: verifyOTP -> $e"); | |
| } catch (e) { | |
| state = state.copyWith(verifying: false, error: e); | |
| state = state.copyWith(verifying: false, actionError: e); | |
| state.copyWith(verifying: true, showErrorVerificationCodeText: false, actionError: null); | |
| try { | |
| await _authService.verifyOTP(countryCode, phoneNumber, state.verificationId!, state.otp); | |
| state = state.copyWith(verifying: false, isVerificationComplete: true); | |
| } on AppError catch (e) { | |
| if (e.l10nCode == AppErrorL10nCodes.invalidVerificationCode) { | |
| state = state.copyWith( | |
| verifying: false, showErrorVerificationCodeText: true); | |
| } else { | |
| state = state.copyWith(verifying: false, actionError: e); | |
| } | |
| debugPrint("PhoneVerificationViewNotifier: error in verifyOTP -> $e"); | |
| } catch (e) { | |
| state = state.copyWith(verifying: false, actionError: e); |
Error Handling through out the app
change verifyOtp text-field
store coutry code and phone number with space in database.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ErrorScreenandPlaceHolderScreenfor better user interaction.showErrorSnackBarfunction to display error messages using localization.Enhancements
Bug Fixes