fix(skilavottord): Fix error message being shown if vehicle is not found for registration #16852
Conversation
WalkthroughThe pull request introduces several modifications primarily focused on enhancing error handling across various components related to vehicle deregistration. Key changes include the addition of specific error handling logic in the GraphQL error link and the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
apps/skilavottord/web/graphql/errorLink.ts (2)
25-28: Improve type safety for error extensionsThe error handling logic is correct and aligns with the PR objective. However, the type casting can be improved.
Consider creating a type for the error response structure:
type GraphQLErrorExtensions = { code?: string; response?: { error?: string; }; }; // Then use it instead of 'any': (graphQLErrors[0]?.extensions as GraphQLErrorExtensions)?.response?.error === 'NOT_FOUND'
25-28: Consider adding user feedback for NOT_FOUND casesWhile silently handling the error prevents unwanted error messages, users might benefit from knowing that the vehicle wasn't found.
Consider showing an informative toast message:
} else if ( errorCodes.includes('FORBIDDEN') || (graphQLErrors[0]?.extensions as any)?.response?.error === 'NOT_FOUND' ) { + if ((graphQLErrors[0]?.extensions as any)?.response?.error === 'NOT_FOUND') { + toast.info('Vehicle not found in the system') + } return }apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx (3)
136-138: Consider improving error handling clarityWhile delegating error handling to ErrorLink is valid, consider these improvements:
- Remove the empty callback since it's not performing any action
- If kept, add type information for better type safety:
onError: (err: ApolloError) => void- Enhance the comment to explain why errors are handled in ErrorLink
- onError: (_err) => { - // Do nothing error handled in ErrorLink - }, + // Errors are handled globally in ErrorLink to prevent UI error messages + // for expected cases like vehicle not found
Line range hint
1-324: Consider architectural improvements for error handling consistencyThe component's error handling could be more consistent and type-safe:
- Standardize error handling approach across queries and mutations
- Use TypeScript discriminated unions for better error type safety
- Consolidate loading states for improved user experience
Consider creating a custom hook to encapsulate the error handling logic:
type QueryResult<T> = { data?: T; loading: boolean; error?: ApolloError; }; function useVehicleQuery(id: string): QueryResult<Vehicle> { const vehicleQuery = useQuery(SkilavottordVehicleReadyToDeregisteredQuery, { variables: { permno: id }, // Errors handled globally in ErrorLink }); const trafficQuery = useQuery(SkilavottordTrafficQuery, { variables: { permno: id }, skip: !vehicleQuery.data, }); return { data: vehicleQuery.data?.skilavottordVehicleReadyToDeregistered, loading: vehicleQuery.loading || trafficQuery.loading, error: vehicleQuery.error || trafficQuery.error, }; }
Line range hint
1-324: Enhance NextJS implementation for better performance and UXConsider these NextJS-specific improvements:
- Implement
getStaticPropsorgetServerSidePropsfor initial data fetching- Add proper loading skeleton UI instead of just loading dots
- Implement error boundaries for better error handling
Example implementation:
// pages/deregister-vehicle/confirm/[id].tsx export const getServerSideProps: GetServerSideProps = async (context) => { const { id } = context.params; const apolloClient = initializeApollo(); try { const { data } = await apolloClient.query({ query: SkilavottordVehicleReadyToDeregisteredQuery, variables: { permno: id }, }); return { props: { initialApolloState: apolloClient.cache.extract(), vehicle: data.skilavottordVehicleReadyToDeregistered, }, }; } catch (error) { return { props: { initialApolloState: apolloClient.cache.extract(), error: error.message, }, }; } };apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
167-172: Improved error handling, but could be more DRYThe changes correctly implement NestJS exception handling and improve error traceability. However, there are a few potential improvements:
- The warning log and error message are duplicative
- The error code could be extracted as a constant for reusability
Consider this refactoring:
+ const NOT_FOUND_ERROR = 'NOT_FOUND'; + const getNotFoundMessage = (loggedPermno: string, partnerId: string) => + `Could not find any requestType for vehicle's number: ${loggedPermno} in database from partner ${partnerId}`; - this.logger.warn( - `car-recycling: Could not find any requestType for vehicle's number: ${loggedPermno} in database from partner ${user.partnerId}`, - ) - throw new NotFoundException( - `Could not find any requestType for vehicle's number: ${loggedPermno} in database from partner ${user.partnerId}`, - 'NOT_FOUND', - ) + const errorMessage = getNotFoundMessage(loggedPermno, user.partnerId); + this.logger.warn(`car-recycling: ${errorMessage}`); + throw new NotFoundException(errorMessage, NOT_FOUND_ERROR);Also applies to: 185-188
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/skilavottord/web/graphql/errorLink.ts(2 hunks)apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx(1 hunks)apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx(1 hunks)apps/skilavottord/web/utils/roleUtils.ts(1 hunks)apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx
- apps/skilavottord/web/utils/roleUtils.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/skilavottord/web/graphql/errorLink.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/DeregisterVehicle/Confirm/Confirm.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
1-6: LGTM! Clean import organization
The addition of NotFoundException follows NestJS best practices for HTTP exception handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16852 +/- ##
==========================================
+ Coverage 35.74% 35.86% +0.12%
==========================================
Files 6937 6898 -39
Lines 148174 146648 -1526
Branches 42253 41785 -468
==========================================
- Hits 52969 52601 -368
+ Misses 95205 94047 -1158
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 661 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
kodiakhq
bot
deleted the
fix/skilavottord-error-shown-after-cancel-deregistration
branch
TS-933
What
Don't show error when canceling de-registration
Fixed typo
Why
Error is being shown when canceling shown if vehicle is not found for de-registration.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores