fix(skilavottord): Improve error handling when creating access and companies #16559
Conversation
WalkthroughThe changes in this pull request enhance error handling for GraphQL mutations related to access control and recycling partner management. Specifically, 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: 3
🧹 Outside diff range and nitpick comments (4)
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (1)
84-90: Good addition of duplicate check, but consider some improvements.The duplicate check effectively prevents "Unhandled Runtime Error" messages and improves user experience. However, consider these enhancements:
- Make the error message more specific by including the nationalId
- Add logging for tracking duplicate attempts
- Move the duplicate check to the service layer for better separation of concerns
Consider this improvement:
- const access = await this.accessControlService.findOne(input.nationalId) - - if (access) { - throw new ConflictException('Access with that national id already exists') - } - - return this.accessControlService.createAccess(input) + return this.accessControlService.createAccess(input, { + logger: this.logger, + errorMessage: `Access already exists for national ID: ${input.nationalId}`, + })Then in the service:
async createAccess( input: CreateAccessControlInput, options?: { logger?: Logger; errorMessage?: string }, ): Promise<AccessControlModel> { const existing = await this.findOne(input.nationalId); if (existing) { options?.logger?.warn('Duplicate access creation attempted', { nationalId: input.nationalId, }); throw new ConflictException( options?.errorMessage ?? 'Access with that national id already exists', ); } return this.create(input); }apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
Line range hint
89-99: Consider centralizing error handling logic.The error handling is currently split between the mutation's
onErrorcallback and the submission handler. Consider consolidating this for better maintainability.Suggestion:
const handleCreateRecyclingPartner = handleSubmit(async (input) => { - const { errors } = await createSkilavottordRecyclingPartner({ - variables: { input: { ...input, active: !!input.active } }, - }) - if (!errors) { - router.push(routes.recyclingCompanies.baseRoute).then(() => { - toast.success(t.recyclingCompany.add.added) - }) - } + try { + const { errors } = await createSkilavottordRecyclingPartner({ + variables: { input: { ...input, active: !!input.active } }, + }) + if (errors?.length) { + toast.error(t.recyclingCompany.add.error) + return + } + await router.push(routes.recyclingCompanies.baseRoute) + toast.success(t.recyclingCompany.add.added) + } catch (error) { + console.error('Failed to create recycling partner:', error) + toast.error(t.recyclingCompany.add.error) + } })apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (2)
Line range hint
164-171: Add error handling to deleteSkilavottordAccessControl mutationFor consistency, add the same error handling pattern to the delete mutation as well.
const [deleteSkilavottordAccessControl] = useMutation( DeleteSkilavottordAccessControlMutation, { + onError(_) { + console.error('Access Control deletion failed:', _) + if (_.networkError) { + // Handle network errors + } else if (_.graphQLErrors?.length) { + // Handle GraphQL errors + } + // Hide Runtime error message. The error message is already shown to the user in toast. + }, refetchQueries: [ { query: SkilavottordAccessControlsQuery, }, ], }, )
Line range hint
1-394: Overall implementation looks good with room for improvementThe component follows NextJS best practices and effectively uses TypeScript. The error handling improvements align with the PR objectives of preventing "Unhandled Runtime Error" messages.
Suggestions for further improvement:
- Consider implementing a centralized error handling utility for consistent error logging and handling across mutations
- Add error boundaries to catch any unhandled errors at the component level
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (2 hunks)
- apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1 hunks)
- apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/skilavottord/web/screens/AccessControl/AccessControl.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/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.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/accessControl/accessControl.resolver.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/accessControl/accessControl.resolver.ts (1)
1-17: LGTM! Import changes align with error handling improvements.The addition of
ConflictExceptionand the organization of imports follow NestJS best practices.
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16559 +/- ##
==========================================
+ Coverage 36.67% 36.76% +0.09%
==========================================
Files 6858 6853 -5
Lines 142787 142027 -760
Branches 40703 40479 -224
==========================================
- Hits 52364 52221 -143
+ Misses 90423 89806 -617
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 135 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 (1)
|
birkirkristmunds
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Line range hint
1-93: Consider standardizing error messages across mutations.While reviewing the error handling, I noticed that
updateSkilavottordRecyclingPartneruses a different error message format compared tocreateSkilavottordRecyclingPartner. For consistency, consider including thecompanyIdin the not found error message as well.- throw new NotFoundException("Recycling partner doesn't exists") + throw new NotFoundException(`Recycling partner with id ${input.companyId} doesn't exist`)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (2 hunks)
- apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.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 (2)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (2)
59-59: LGTM! Improved error message clarity.The error message now includes the specific
companyId, making it easier to identify which recycling partner caused the conflict.
Line range hint
1-93: Verify error handling in the frontend.Since this PR aims to improve error handling and prevent "Unhandled Runtime Error" messages, ensure that the frontend properly handles these exceptions through toast notifications.
✅ Verification successful
Error handling is properly implemented in the frontend
The codebase has comprehensive error handling for GraphQL operations:
- A global error link (
errorLink.ts) handles all GraphQL errors and network errors, displaying them via toast notifications- The Apollo client is configured to show error messages through toast notifications for both network errors and GraphQL errors
- Authentication/authorization errors are properly handled with appropriate redirects
- The
createSkilavottordRecyclingPartnermutation is used within a proper error handling context, where errors will be caught and displayed through the global error handlerThe implementation ensures that the "Unhandled Runtime Error" messages will be properly caught and displayed to users through toast notifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GraphQL mutation usage in frontend components rg -t typescript -t tsx "createSkilavottordRecyclingPartner.*mutation" apps/skilavottord/web # Search for error handling patterns rg -t typescript -t tsx "onError.*toast" apps/skilavottord/webLength of output: 229
Script:
#!/bin/bash # Search for GraphQL mutation usage in frontend components without file type filters rg "createSkilavottordRecyclingPartner.*mutation" apps/skilavottord/web # Search for error handling patterns rg "onError.*toast" apps/skilavottord/web # Search for any toast-related error handling rg "toast.*error" apps/skilavottord/web # Look for mutation error handling patterns rg "mutation.*catch" apps/skilavottord/web # Search for GraphQL error handling patterns rg "GraphQLError|ApolloError" apps/skilavottord/webLength of output: 715
Script:
#!/bin/bash # Check the error handling setup in Apollo client configuration cat apps/skilavottord/web/graphql/errorLink.ts # Look for the actual mutation implementation rg -B2 -A2 "createSkilavottordRecyclingPartner" apps/skilavottord/webLength of output: 3457
birkirkristmunds
changed the title
birkirkristmunds
added
the
deprecated:automerge
…-when-creating-access-and-companies
kodiakhq
bot
deleted the
chore/skilavottord-add-better-error-handling-when-creating-access-and-companies
branch
TS-931
Attach a link to issue if relevant
What
Improve the error handling when registering access and recycling companies
Why
Prevent showing the user Unhandled Runtime Error in the UI
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes