feat: create common graphql error handler #384
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the notifications and providers management system. The changes focus on improving data retrieval, error handling, and user interface for managing providers and notifications. New GraphQL queries have been added to fetch providers and notifications together, with support for both active and archived notifications. The modifications include updates to services, components, and models to support more flexible and type-safe data fetching and filtering. Changes
Sequence DiagramsequenceDiagram
participant User
participant NotificationsComponent
participant ProvidersService
participant GraphQLService
User->>NotificationsComponent: Select Application
NotificationsComponent->>ProvidersService: getProvidersAndNotifications()
ProvidersService->>GraphQLService: Execute GraphQL Query
GraphQLService-->>ProvidersService: Return Query Results
ProvidersService-->>NotificationsComponent: Provide Providers and Notifications
NotificationsComponent->>User: Display Providers and Notifications
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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 comments (1)
apps/portal/src/app/views/applications/applications.service.ts (1)
Line range hint
22-38: Use the common error handler for consistency.The service should use the new
validateApolloErrorsmethod for consistent error handling across the application.Apply this change:
getApplications(variables: unknown, inputToken: string): Observable<ApplicationResponse> { return this.graphqlService.query(GetApplications, variables, inputToken).pipe( map((response: ApolloQueryResult<GetApplicationsResponse>) => { + this.graphqlService.validateApolloErrors(response); const applicationArray = response.data?.applications.applications; const applicationResponseObject: ApplicationResponse = { applications: applicationArray, total: response.data?.applications.total, offset: response.data?.applications.offset, limit: response.data?.applications.limit, - errors: response.errors || null, // Remove this as errors are now handled by validateApolloErrors }; return applicationResponseObject; }), catchError((error) => { const errorMessage: string = error.message; throw new Error(errorMessage); }), ); }
🧹 Nitpick comments (11)
apps/portal/src/app/views/providers/providers.service.ts (2)
48-95: Consider extracting common code for the archived vs non-archived branches.
Theif (archivedNotificationToggle)branch duplicates logic (mapping, error handling, etc.). You could reduce duplication by consolidating transformations in a helper function.Sample approach:
if (archivedNotificationToggle) { // ... } else { // ... } + // Potentially unify repeated logic in a shared callback that converts + // providers and notifications (or archived notifications) to the standard form + // and handles the error and pipe.
97-120: Catch error objects without losing context.
When catching errors, re-throwing them as new Error objects can lose stack traces or additional info. Consider re-throwing the original error or attaching the original data to preserve debugging context.apps/portal/src/app/views/notifications/notifications.component.ts (3)
48-48: Initialize providers with stronger type.
If you intend providers to always hold an array of typed provider objects, explicitly declare the array type to improve maintainability and clarity.- providers = []; + providers: { label: string; value: number }[] = [];
Line range hint
102-179: Avoid deeply nested subscriptions in production code.
While this is acceptable, consider organizing load steps so each call unsubscribes properly or merges streams elegantly (e.g., using RxJSswitchMap) to reduce the chance of memory leaks.
384-386: Filters in multiple places.
You have multiple filter sets (inloadProvidersAndNotificationsForSelectedApplicationand inloadNotificationsLazy). Consider centralizing or reusing filter logic to avoid duplication.apps/portal/src/app/views/providers/provider.model.ts (1)
1-11: Prefer enumerations where applicable.
channelTypeandstatusare typed asnumber; for maintainability, consider using a TypeScriptenumor union for clarity and safer references.apps/portal/src/app/graphql/graphql.service.ts (1)
63-73: Consider enhancing error types and handling.The error handler implementation is good but could be improved with more specific error types and additional context.
Consider these enhancements:
- validateApolloErrors<T>(response: ApolloQueryResult<T>): void { + validateApolloErrors<T>(response: ApolloQueryResult<T>): void { + interface GraphQLError { + code?: string; + message: string; + path?: readonly (string | number)[]; + } + if (response.errors && response.errors.length > 0) { - const errorMessages = response.errors.map((err) => err.message).join('; '); + const errorMessages = response.errors.map((err: GraphQLError) => + `${err.code ? `[${err.code}] ` : ''}${err.message}${err.path ? ` at ${err.path.join('.')}` : ''}` + ).join('; '); throw new Error(`GraphQL Formatted Error(s): ${errorMessages}`); } if (response.error) { - throw new Error(`Apollo Error: ${response.error.message}`); + throw new Error(`Apollo Network Error: ${response.error.message}`); } }apps/portal/src/app/graphql/graphql.queries.ts (2)
97-154: Consider using GraphQL fragments to reduce duplication.The provider and notification field selections are duplicated across queries. Using fragments would improve maintainability.
Consider refactoring to use fragments:
+const ProviderFields = gql` + fragment ProviderFields on Provider { + applicationId + channelType + createdOn + name + providerId + status + updatedOn + } +`; + +const NotificationFields = gql` + fragment NotificationFields on Notification { + channelType + createdBy + createdOn + data + deliveryStatus + id + result + status + updatedBy + updatedOn + } +`; export const GetProvidersAndNotifications = gql` + ${ProviderFields} + ${NotificationFields} query GetProvidersAndNotifications( $providerLimit: Int! # ... other variables ) { providers( options: { # ... options } ) { providers { - applicationId - channelType - # ... other fields + ...ProviderFields } # ... pagination fields } notifications { notifications { - channelType - createdBy - # ... other fields + ...NotificationFields } # ... pagination fields } } `;
156-213: Consider making sort order configurable.The sort orders are hardcoded (ASC for providers, DESC for notifications). Consider making them configurable via variables.
Consider this enhancement:
export const GetProvidersAndArchivedNotifications = gql` query GetProvidersAndArchivedNotifications( $providerLimit: Int! $providerOffset: Int! $providerFilters: [UniversalFilter!] + $providerSortOrder: SortOrder = ASC $notificationLimit: Int! $notificationOffset: Int! $notificationFilters: [UniversalFilter!] + $notificationSortOrder: SortOrder = DESC ) { providers( options: { limit: $providerLimit offset: $providerOffset sortBy: "createdOn" - sortOrder: ASC + sortOrder: $providerSortOrder filters: $providerFilters } ) # ... rest of the query } `;apps/portal/src/app/views/notifications/notifications.service.ts (1)
Line range hint
32-83: Extract common error handling logic.Both methods have identical error handling logic. Consider extracting it to a private method.
Consider this refactor:
+ private handleGraphQLError(error: Error): never { + const errorMessage: string = error.message; + throw new Error(errorMessage); + } + getNotifications(variables: unknown, inputToken: string): Observable<NotificationResponse> { return this.graphqlService.query(GetNotifications, variables, inputToken).pipe( map((response: ApolloQueryResult<GetNotificationsResponse>) => { this.graphqlService.validateApolloErrors(response); // ... rest of the mapping logic }), - catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); - }), + catchError(this.handleGraphQLError.bind(this)), ); } getArchivedNotifications(variables: unknown, inputToken: string): Observable<NotificationResponse> { return this.graphqlService.query(GetArchivedNotifications, variables, inputToken).pipe( map((response: ApolloQueryResult<GetArchivedNotificationsResponse>) => { this.graphqlService.validateApolloErrors(response); // ... rest of the mapping logic }), - catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); - }), + catchError(this.handleGraphQLError.bind(this)), ); }apps/portal/src/app/views/notifications/notifications.component.html (1)
55-57: LGTM! Consider caching providers.The change to load providers and notifications together is a good improvement. However, since providers data might not change as frequently as notifications, consider caching the providers data to reduce unnecessary API calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)apps/portal/src/app/graphql/graphql.service.ts(1 hunks)apps/portal/src/app/views/applications/applications.service.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.html(2 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(9 hunks)apps/portal/src/app/views/notifications/notifications.service.ts(2 hunks)apps/portal/src/app/views/providers/provider.model.ts(1 hunks)apps/portal/src/app/views/providers/providers.service.ts(1 hunks)
🔇 Additional comments (11)
apps/portal/src/app/views/providers/providers.service.ts (3)
1-11: Use consistent import paths or naming if possible.
Everything here looks consistent and follows Angular style norms. No issues found.
12-25: Confirm interfaces reflect actual GraphQL data shape.
These fields, especially optional ones, should reflect how the backend resolves the data. Be sure all properties inGetProviderNotificationResponseexist in the GraphQL schema to prevent runtime errors.
27-40: Align archived notifications interface with GraphQL schema.
Similar to the above comment, ensureGetProviderArchivedNotificationResponseprecisely matches the shape of the archived notifications response (naming, optional vs required).apps/portal/src/app/views/notifications/notifications.component.ts (6)
10-11: Imports look good.
No concerns with these additional imports.
54-55: Selected provider defaults.
SettingselectedProvidertonullis fine. Just confirm that no further references causeundefinedchecks in the template.
87-87: Service injection is consistent.
Good job injectingProvidersServicefor use across the component.
93-93: Ensure no stale references to old function name.
Renaming togetApplicationsAndInitializeData()is clear. Just confirm any references togetApplications()or old references are replaced.
228-350: Guard against partial or null data inproviderAndNotificationResponse.
When referencing arrays likeprovidersornotifications, ensure you null-check them before mapping to avoid potential runtime errors.
456-505: Check date filter logic inclusivity.
Lines 475–493 show date range filtering. Confirm that shifting dates (like+1forselectedToDate) meets your inclusive/exclusive logic (e.g., do you want “less than” vs “less than or equal”?).apps/portal/src/app/views/providers/provider.model.ts (1)
13-23: Interfaces look consistent with the rest of the code.
No issues found. They align with the usage in ProvidersService.apps/portal/src/app/views/notifications/notifications.component.html (1)
9-11: Verify loadNotificationsLazy implementation for provider filtering.The dropdown has been updated to use providers instead of channel types. Please ensure that the
loadNotificationsLazymethod in the component class has been updated to handle the newselectedProvidermodel correctly.Run the following script to verify the implementation:
✅ Verification successful
Implementation correctly handles provider filtering ✅
The
loadNotificationsLazymethod properly integrates with the new provider dropdown through thegetCombinedNotificationFiltersmethod, which correctly adds the provider filter whenselectedProvideris set.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that loadNotificationsLazy has been updated to handle selectedProvider. # Test: Search for loadNotificationsLazy implementation ast-grep --pattern $'loadNotificationsLazy($params) { $$$ }'Length of output: 63
Script:
#!/bin/bash # Find the component's TypeScript file fd "notifications.component.ts" # Search for loadNotificationsLazy implementation in TypeScript files rg "loadNotificationsLazy" -t ts -A 10Length of output: 2065
Script:
#!/bin/bash # Get the complete implementation of loadNotificationsLazy rg "loadNotificationsLazy" -t ts -A 30 apps/portal/src/app/views/notifications/notifications.component.tsLength of output: 1729
Script:
#!/bin/bash # Get more context lines to see complete filter construction rg "loadNotificationsLazy" -t ts -B 5 -A 50 apps/portal/src/app/views/notifications/notifications.component.tsLength of output: 3365
Script:
#!/bin/bash # Find the getCombinedNotificationFilters implementation rg "getCombinedNotificationFilters" -t ts -A 20 apps/portal/src/app/views/notifications/notifications.component.tsLength of output: 2093
|
Keeping this PR on hold. @kshitij-k-osmosys when you are back kindly resolve conflicts. |
Portal PR Checklist
Task Link
OSXT-234
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login page)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Create common graphql error handler
validateApolloErrors(response)Add the error handler before service functions process data
OnInitialization, applications already has error handling here so did not add it
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes