feat: fetch provider list on portal based on selected application #373
Conversation
WalkthroughThe pull request introduces two new GraphQL queries for retrieving providers and notifications, enhancing the notifications component's filtering capabilities. It replaces the channel type selection with a provider selection dropdown, modifies the event handling for application selection, and integrates a new service for fetching provider-related data. Additionally, new TypeScript interfaces are defined to structure the provider and notification responses, improving type safety and clarity in data management. Changes
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: 2
🧹 Nitpick comments (4)
apps/portal/src/app/views/notifications/notifications.component.ts (2)
260-265: Avoid displaying technical error details to usersDisplaying raw error messages can expose sensitive information. Consider showing a generic error message and logging the detailed error for internal debugging.
Apply this diff to modify the error message:
this.messageService.add({ key: 'tst', severity: 'error', summary: 'Error', - detail: `There was an error while loading providers. Reason: ${error.message}`, + detail: 'There was an error while loading providers. Please try again later.', });
231-307: Refactor duplicated error handling logicThe error handling code in
getProvidersForSelectedApplication()is similar to the one ingetApplications(). Consider abstracting this logic into a shared method to reduce code duplication and enhance maintainability.apps/portal/src/app/views/providers/providers.service.ts (1)
36-40: Simplify error handling in the serviceCatching and rethrowing the error without additional processing is unnecessary and might obscure the original stack trace. Consider removing the
catchErrorblock to allow errors to propagate naturally.Apply this diff to remove unnecessary error handling:
}), - catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); - }), );apps/portal/src/app/views/notifications/notifications.component.html (1)
9-11: Add accessibility attributes to the provider dropdown.While the provider dropdown implementation is correct, consider enhancing accessibility by adding appropriate ARIA attributes and a visible label.
<p-dropdown + [attr.aria-label]="'Select notification provider'" + [ariaLabelledBy]="'provider-dropdown-label'" [options]="providers" [(ngModel)]="selectedProvider" placeholder="Select a Provider" class="grid-dropdown" [showClear]="true" (onChange)="loadNotificationsLazy({ first: 0, rows: this.pageSize })" ></p-dropdown>Consider adding a visible label above the dropdown:
+<label id="provider-dropdown-label" class="dropdown-label">Provider</label> <p-dropdown
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.html(2 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(6 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 (3)
apps/portal/src/app/views/providers/provider.model.ts (1)
1-18: Interfaces are well-defined
The Provider and ProviderResponse interfaces are correctly defined and follow TypeScript best practices.
apps/portal/src/app/views/notifications/notifications.component.html (2)
55-58: 🛠️ Refactor suggestion
Consider optimizing the application selection handler.
The current implementation has potential issues:
- Loading notifications before updating the providers list could lead to inconsistent state
- The template syntax for multiple statements could be simplified
Consider refactoring to:
- Update providers first, then load notifications
- Move the logic to a component method
- (onChange)="
- loadNotificationsLazy({ first: 0, rows: this.pageSize });
- getProvidersForSelectedApplication()
- "
+ (onChange)="onApplicationChange()"Add in your component:
async onApplicationChange() {
try {
await this.getProvidersForSelectedApplication();
await this.loadNotificationsLazy({ first: 0, rows: this.pageSize });
} catch (error) {
// Handle error appropriately
}
}Let's verify the error handling in getProvidersForSelectedApplication:
Line range hint 134-156: Verify the relationship between providers and channel types.
The table still uses channelTypeMap for display logic while the filtering has moved to providers. This suggests a potential disconnect between the filtering and display logic.
Let's verify the relationship between providers and channel types:
✅ Verification successful
Based on the codebase analysis, I can now generate the final response:
The channel type mapping is correctly integrated with providers
The relationship between providers and channel types is properly maintained. The channelTypeMap is used to display provider-specific UI elements (icons, styles, alt text) while the component also handles provider selection based on channel types. This is evidenced by:
- The
ChannelTypeMapconstant defines display properties for each channel type - The component uses this mapping to format provider labels:
${obj.name} - ${this.channelTypeMap[obj.channelType].altText} - The backend
Providerentity enforces channel type validation through@IsEnum(ChannelType)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the relationship between providers and channel types
rg -A 10 "channelTypeMap|ChannelType|Provider" --type typescript
Length of output: 101
Script:
#!/bin/bash
# Search for the relationship between providers and channel types with .ts extension
rg -A 10 "channelTypeMap|ChannelType|Provider" --type ts
Length of output: 65595
Script:
#!/bin/bash
# Let's check the component's typescript file to understand the relationship between providers and channel types
rg -A 10 "channelTypeMap|notifications\.component" --type ts apps/portal/src/app/views/notifications/
Length of output: 5431
| getProvidersForSelectedApplication() { | ||
| // Set the query variables | ||
| const variables = { | ||
| filters: [], | ||
| offset: 0, | ||
| limit: 15, | ||
| }; | ||
|
|
||
| // Set query filters | ||
| variables.filters.push({ | ||
| field: 'applicationId', | ||
| operator: 'eq', | ||
| value: this.selectedApplication.toString(), | ||
| }); |
There was a problem hiding this comment.
Add null check for selectedApplication before using it
To prevent potential runtime errors, ensure that this.selectedApplication is not null before calling .toString() and using it in filters.
Apply this diff to add a null check:
+ if (!this.selectedApplication) {
+ // Handle missing selected application
+ return;
+ }
variables.filters.push({
field: 'applicationId',
operator: 'eq',
value: this.selectedApplication.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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getProvidersForSelectedApplication() { | |
| // Set the query variables | |
| const variables = { | |
| filters: [], | |
| offset: 0, | |
| limit: 15, | |
| }; | |
| // Set query filters | |
| variables.filters.push({ | |
| field: 'applicationId', | |
| operator: 'eq', | |
| value: this.selectedApplication.toString(), | |
| }); | |
| getProvidersForSelectedApplication() { | |
| // Set the query variables | |
| const variables = { | |
| filters: [], | |
| offset: 0, | |
| limit: 15, | |
| }; | |
| // Set query filters | |
| if (!this.selectedApplication) { | |
| // Handle missing selected application | |
| return; | |
| } | |
| variables.filters.push({ | |
| field: 'applicationId', | |
| operator: 'eq', | |
| value: this.selectedApplication.toString(), | |
| }); |
There was a problem hiding this comment.
@kshitij-k-osmosys Do we have to do this? Add the change if needed.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Add a fix here 220abdb
After adding a null check, since notifications are loaded before providers, all that this check does is to not populate provider dropdown
SO notifications get loaded, but provider dropdown is empty
Should I a do a logout for it?
There was a problem hiding this comment.
No need to logout.
So notifications get loaded, but provider dropdown is empty => This should never happen if there are notifications present.
So make sure code is handled properly. You need to handle updating Provider/Notification sync properly. So you can do this => Do a graphQL Call which adds two queries in a single call to fetch notifications and providers when loading initially.
Can add a function which can take the query list and do combined query call instead.
@coderabbitai. Can you help with this?
There was a problem hiding this comment.
@xixas, here's an approach you can try to fetch both providers and notifications in one go, ensuring they remain in sync and avoiding an empty provider dropdown when notifications exist.
- Create a combined GraphQL query to retrieve both providers and notifications together (with their respective filters, offsets, and limits). For instance, you might define a new query in your GraphQL schema file:
typescript code block:
import { gql } from 'apollo-angular';
export const GET_PROVIDERS_AND_NOTIFICATIONS = gqlquery GetProvidersAndNotifications( $providerFilters: [FilterInput!] $providerOffset: Int $providerLimit: Int $notificationFilters: [FilterInput!] $notificationOffset: Int $notificationLimit: Int ) { providers( filters: $providerFilters offset: $providerOffset limit: $providerLimit sortBy: "createdOn" sortOrder: ASC ) { providers { providerId name channelType createdOn updatedOn status } total offset limit } notifications( filters: $notificationFilters offset: $notificationOffset limit: $notificationLimit ) { notifications { notificationId createdOn ... } total offset limit } };
- In a service (for example, NotificationsService or a dedicated CombinedService), create a method that calls this new query:
typescript code block:
import { Injectable } from '@angular/core';
import { Apollo, ApolloQueryResult } from 'apollo-angular';
import { GET_PROVIDERS_AND_NOTIFICATIONS } from 'src/app/graphql/graphql.queries';
import { map } from 'rxjs/operators';
import { Observable } from 'rxjs';
@Injectable({
providedIn: 'root',
})
export class CombinedNotificationsService {
constructor(private apollo: Apollo) {}
getProvidersAndNotifications(
variables: {
providerFilters: any[];
providerOffset: number;
providerLimit: number;
notificationFilters: any[];
notificationOffset: number;
notificationLimit: number;
},
inputToken: string,
): Observable<{
providers: any;
notifications: any;
}> {
return this.apollo
.query({
query: GET_PROVIDERS_AND_NOTIFICATIONS,
variables,
context: {
// For example, passing the token if your GraphQL setup uses headers.
headers: {
Authorization: Bearer ${inputToken},
},
},
fetchPolicy: 'network-only',
})
.pipe(
map((result: ApolloQueryResult) => {
const data = result.data;
return {
providers: data.providers,
notifications: data.notifications,
};
}),
);
}
}
- In your component (e.g. NotificationsComponent), you can replace the separate calls to getProviders(...) and getNotifications(...) with a single subscription to getProvidersAndNotifications(...). That way, you’ll receive both lists at once and can ensure that providers are populated whenever notifications are present:
typescript code block:
const loginToken = this.getJWTLoginToken();
if (!loginToken) {
return;
}
const variables = {
providerFilters: [
{
field: 'applicationId',
operator: 'eq',
value: this.selectedApplication?.toString(),
},
],
providerOffset: 0,
providerLimit: 15,
notificationFilters: [
{
field: 'applicationId',
operator: 'eq',
value: this.selectedApplication?.toString(),
},
// Additional filters for date range, status, etc.
],
notificationOffset: 0,
notificationLimit: 10,
};
this.combinedNotificationsService
.getProvidersAndNotifications(variables, loginToken)
.subscribe((result) => {
if (result.providers?.providers) {
this.providers = result.providers.providers.map((obj) => ({
label: ${obj.name} - ${this.channelTypeMap[obj.channelType].altText},
value: obj.providerId,
}));
}
if (result.notifications?.notifications) {
this.notifications = result.notifications.notifications;
this.totalRecords = result.notifications.total;
}
});
This ensures:
• If there are valid notifications, you’ll also have the corresponding providers.
• You minimize the risk of an empty dropdown whenever notifications exist.
• You avoid duplicating checks and performing repeated sequential queries.
Give that a try! It’s often easier to keep providers and notifications in sync if you fetch them with a single query whenever possible.
There was a problem hiding this comment.
@kshitij-k-osmosys Check this solution but don't do it right now in this PR if the case doesn't happen. Can keep as TODO.
There was a problem hiding this comment.
Ok, already have added it here however ff6011d
| export const GetProviders = gql` | ||
| query GetProviders($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) { | ||
| providers( | ||
| options: { | ||
| limit: $limit | ||
| offset: $offset | ||
| sortBy: "createdOn" | ||
| sortOrder: ASC | ||
| filters: $filters | ||
| } | ||
| ) { | ||
| providers { | ||
| channelType | ||
| createdOn | ||
| name | ||
| providerId | ||
| status | ||
| updatedOn | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
Include pagination fields in GetProviders GraphQL query
The GetProviders query does not retrieve total, offset, and limit fields, which are expected by ProvidersService. This may lead to undefined values in the response and potential errors.
Apply this diff to include pagination fields in the query:
}
+ total
+ offset
+ limit
}
}
`;📝 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.
| export const GetProviders = gql` | |
| query GetProviders($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) { | |
| providers( | |
| options: { | |
| limit: $limit | |
| offset: $offset | |
| sortBy: "createdOn" | |
| sortOrder: ASC | |
| filters: $filters | |
| } | |
| ) { | |
| providers { | |
| channelType | |
| createdOn | |
| name | |
| providerId | |
| status | |
| updatedOn | |
| } | |
| } | |
| } | |
| `; | |
| export const GetProviders = gql` | |
| query GetProviders($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) { | |
| providers( | |
| options: { | |
| limit: $limit | |
| offset: $offset | |
| sortBy: "createdOn" | |
| sortOrder: ASC | |
| filters: $filters | |
| } | |
| ) { | |
| providers { | |
| channelType | |
| createdOn | |
| name | |
| providerId | |
| status | |
| updatedOn | |
| } | |
| total | |
| offset | |
| limit | |
| } | |
| } | |
| `; |
There was a problem hiding this comment.
fixed bfb40f6
There was a problem hiding this comment.
@kshitij-k-osmosys, thank you for fixing the issue!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)
🔇 Additional comments (1)
apps/portal/src/app/graphql/graphql.queries.ts (1)
89-113: LGTM! Query structure follows best practices.
The GetProviders query is well-structured and consistent with other queries in the file. It properly includes pagination, sorting, and filtering capabilities.
| providers { | ||
| channelType | ||
| createdOn | ||
| name | ||
| providerId | ||
| status | ||
| updatedOn | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
The applicationId field should be included in the providers response as it's a defined field in the Provider entity
The Provider entity in apps/api/src/modules/providers/entities/provider.entity.ts clearly shows that applicationId is a required field decorated with @Field() for GraphQL exposure. This field is essential for establishing the relationship between providers and applications, and it should be included in the providers response.
- Location:
apps/portal/src/app/graphql/graphql.queries.ts:100-107
🔗 Analysis chain
Consider adding applicationId field to providers response.
Given that the PR objective is to "fetch provider list based on selected application", the providers response should include an applicationId field to establish the relationship between providers and applications.
Apply this diff to include the applicationId field:
providers {
channelType
createdOn
name
providerId
status
updatedOn
+ applicationId
}Let me run one more verification to check the Provider type definition since the previous results don't show it.
Let me try a different approach to find the Provider type definition.
Let me check one more thing to see if the Provider entity has an applicationId field that might not be visible in the first few lines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if applicationId is defined in the Provider type and used in the codebase
# Check if Provider type includes applicationId
ast-grep --pattern 'type Provider {
$$$
applicationId: $_
$$$
}'
# Check for applicationId usage in provider-related code
rg -A 5 'applicationId.*provider'
Length of output: 30563
Script:
#!/bin/bash
# Search for Provider type definition in GraphQL schema files
fd -e graphql -e ts -x cat {} | grep -A 10 "type Provider"
Length of output: 62
Script:
#!/bin/bash
# Search for Provider type definition using ast-grep and ripgrep
ast-grep --pattern 'type Provider {
$$$
}'
# Also search for Provider interface or class definition
rg "(?:type|interface|class)\s+Provider\s*\{" -A 10
Length of output: 1806
Script:
#!/bin/bash
# Get the complete Provider entity definition
cat apps/api/src/modules/providers/entities/provider.entity.ts
Length of output: 2127
There was a problem hiding this comment.
@xixas, thank you for addressing this!
There was a problem hiding this comment.
fixed e4e9993
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
fixed e4e9993
| // Fetch the login token | ||
| const loginToken = this.getJWTLoginToken(); | ||
|
|
||
| if (!loginToken) { |
There was a problem hiding this comment.
If there is no login token we should be logging out.
There was a problem hiding this comment.
Assuming its handled in getJWTLoginToken()
There was a problem hiding this comment.
It is not being handled in getJWTLoginToken()
However we do have a logout function, maybe we can create new PR for it
There was a problem hiding this comment.
@kshitij-k-osmosys Okay can do in New PR.
| .getProviders(variables, loginToken) | ||
| .pipe( | ||
| // catchError operator to handle errors | ||
| catchError((error) => { |
There was a problem hiding this comment.
We don't have common code for this code block? Can add a TODO task for this
There was a problem hiding this comment.
Should we create an issue?
There was a problem hiding this comment.
@kshitij-k-osmosys No need, directly create a task in pinestem for now in backlog.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/portal/src/app/views/notifications/notifications.component.ts (2)
264-272: Consider extracting common error handling logicThis error handling block is duplicated across multiple methods in the component. Consider extracting it into a reusable method.
+ private handleServiceError(errorMessage: string) { + this.messageService.add({ + key: 'tst', + severity: 'error', + summary: 'Error', + detail: `There was an error while ${errorMessage}`, + }); + return of(null); + } getProvidersForSelectedApplication() { // ... .pipe( - catchError((error) => { - this.messageService.add({ - key: 'tst', - severity: 'error', - summary: 'Error', - detail: `There was an error while loading providers. Reason: ${error.message}`, - }); - return of(null); - }), + catchError((error) => this.handleServiceError(`loading providers. Reason: ${error.message}`)), )
282-290: Extract common unauthorized error handlingThe unauthorized error handling logic is duplicated. Consider extracting it into a shared method.
+ private handleUnauthorizedError() { + this.messageService.add({ + key: 'tst', + severity: 'error', + summary: 'Error', + detail: 'Unauthorized access. Please log in again.', + }); + this.authService.logoutUser(); + } if (unauthorizedError) { - this.messageService.add({ - key: 'tst', - severity: 'error', - summary: 'Error', - detail: 'Unauthorized access. Please log in again.', - }); - this.authService.logoutUser(); + this.handleUnauthorizedError(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/app/views/notifications/notifications.component.ts(6 hunks)
🔇 Additional comments (4)
apps/portal/src/app/views/notifications/notifications.component.ts (4)
10-11: LGTM: New imports and properties are well-defined
The provider-related imports and class properties are correctly implemented with appropriate initialization values.
Also applies to: 48-48, 54-54
87-87: LGTM: Service injection is properly implemented
The ProvidersService is correctly injected following Angular's dependency injection pattern.
180-182: LGTM: Provider initialization is properly sequenced
The provider loading is correctly triggered after applications are loaded, maintaining proper dependency order.
354-360: LGTM: Provider filtering is properly implemented
The provider filter is correctly integrated into the existing filtering system with proper null checks.
| // Fetch list of providers for dropdown | ||
| this.providers = providerResponse.providers.map((obj) => ({ | ||
| // Name to display and ID to return upon selection | ||
| label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`, |
There was a problem hiding this comment.
Add null check for channelType mapping
The code assumes that the provider's channelType will always exist in channelTypeMap. Add a null check to handle potential missing mappings.
- label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`,
+ label: `${obj.name}${obj.channelType && this.channelTypeMap[obj.channelType]?.altText ? ` - ${this.channelTypeMap[obj.channelType].altText}` : ''}`,📝 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.
| label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`, | |
| label: `${obj.name}${obj.channelType && this.channelTypeMap[obj.channelType]?.altText ? ` - ${this.channelTypeMap[obj.channelType].altText}` : ''}`, |
There was a problem hiding this comment.
fixed 4fd8b8f
There was a problem hiding this comment.
@kshitij-k-osmosys Thank you for implementing the fix! Your solution with the fallback to ChannelType.UNKNOWN is even better than my suggestion as it ensures there's always a meaningful label displayed. Well done!
🔗 Analysis chain
@kshitij-k-osmosys Thank you for implementing the fix! The code is now more robust in handling missing channel type mappings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the specific commit details
git show 4fd8b8ff412c14a6cf8bb85b304dbab3a6ffbf94
Length of output: 1212
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| .getProviders(variables, loginToken) | ||
| .pipe( | ||
| // catchError operator to handle errors | ||
| catchError((error) => { |
There was a problem hiding this comment.
@kshitij-k-osmosys No need, directly create a task in pinestem for now in backlog.
| // Fetch the login token | ||
| const loginToken = this.getJWTLoginToken(); | ||
|
|
||
| if (!loginToken) { |
There was a problem hiding this comment.
@kshitij-k-osmosys Okay can do in New PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/portal/src/app/views/notifications/notifications.component.ts (1)
304-304:⚠️ Potential issueAdd null check for channelType mapping.
The code assumes that the provider's channelType will always exist in channelTypeMap. Add a null check to handle potential missing mappings.
- label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`, + label: `${obj.name}${obj.channelType && this.channelTypeMap[obj.channelType]?.altText ? ` - ${this.channelTypeMap[obj.channelType].altText}` : ''}`,
🧹 Nitpick comments (1)
apps/portal/src/app/views/notifications/notifications.component.ts (1)
180-182: Consider optimizing the data loading sequence.Currently, notifications and providers are loaded independently after applications are fetched. This could lead to a race condition where notifications are displayed before providers are loaded. Consider using a combined query as suggested in the previous review or using RxJS operators to coordinate these calls.
Example using RxJS:
import { forkJoin } from 'rxjs'; // Inside getApplications() success handler if (this.applications.length > 0) { this.selectedApplication = this.applications[0].value; // Load both notifications and providers in parallel forkJoin({ notifications: this.loadNotificationsLazy({ first: 0, rows: this.pageSize }), providers: this.getProvidersForSelectedApplication() }).subscribe({ next: (results) => { // Handle results }, error: (error) => { // Handle error } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/app/views/notifications/notifications.component.ts(6 hunks)
🔇 Additional comments (3)
apps/portal/src/app/views/notifications/notifications.component.ts (3)
10-11: LGTM: Import statements and class properties are well-structured.
The new imports and class properties are properly defined and follow Angular conventions.
Also applies to: 48-48, 54-54
87-87: LGTM: Proper dependency injection.
The ProvidersService is correctly injected following Angular's dependency injection pattern.
354-360: LGTM: Provider filtering is properly implemented.
The provider filter is correctly added to the notifications query, following the same pattern as other filters.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/portal/src/app/views/notifications/notifications.component.html (1)
Line range hint
134-186: Clarify the relationship between providers and channel types in the UI.While the filtering has been updated to use providers instead of channel types, the table still displays channel type information. Please clarify:
- Is this intentional?
- What is the relationship between providers and channel types?
- Should the channel type display be updated to show provider information instead?
Consider documenting the relationship between providers and channel types in the component's documentation to help future maintainers understand the design decision.
apps/portal/src/app/views/providers/provider.model.ts (1)
4-11: Consider enhancing the Provider interface with validation decoratorsThe interface could benefit from validation decorators to ensure data integrity. Consider adding:
@IsNumber()for providerId@IsString()for name@IsNumber()for channelType and status@IsDate()for date fieldsapps/portal/src/app/graphql/graphql.queries.ts (1)
97-154: Consider using GraphQL fragments to reduce duplicationThe provider fields are duplicated across both queries. Consider extracting them into a shared fragment:
+const ProviderFields = gql` + fragment ProviderFields on Provider { + applicationId + channelType + createdOn + name + providerId + status + updatedOn + } `; export const GetProvidersAndNotifications = gql` query GetProvidersAndNotifications( $providerLimit: Int! $providerOffset: Int! $providerFilters: [UniversalFilter!] $notificationLimit: Int! $notificationOffset: Int! $notificationFilters: [UniversalFilter!] ) { providers( options: { limit: $providerLimit offset: $providerOffset sortBy: "createdOn" sortOrder: ASC filters: $providerFilters } ) { providers { - applicationId - channelType - createdOn - name - providerId - status - updatedOn + ...ProviderFields } total offset limit } # ... rest of the query } ${ProviderFields} `;apps/portal/src/app/views/providers/providers.service.ts (2)
89-93: Enhance error handling with specific error typesThe current error handling simply wraps the message in a new Error. Consider:
- Creating specific error types for different failure scenarios
- Adding error context for debugging
+class ProviderServiceError extends Error { + constructor(message: string, public readonly cause: unknown) { + super(message); + this.name = 'ProviderServiceError'; + } +} catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); + throw new ProviderServiceError( + `Failed to fetch providers and notifications: ${error.message}`, + error + ); })Also applies to: 114-118
62-75: Consider extracting the notification mapping logicThe notification mapping logic could be moved to a separate method for better maintainability:
+ private mapArchivedToNotification( + archived: ArchivedNotification + ): Notification { + return { + id: archived.notificationId, + channelType: archived.channelType, + data: archived.data, + deliveryStatus: archived.deliveryStatus, + result: archived.result, + createdOn: archived.createdOn, + updatedOn: archived.updatedOn, + createdBy: archived.createdBy, + updatedBy: archived.updatedBy, + status: archived.status, + }; + } - const convertToNotificationArray = - archivedNotificationArray?.map((item) => ({ - id: item.notificationId, - // ... rest of mapping - })) ?? []; + const convertToNotificationArray = + archivedNotificationArray?.map(this.mapArchivedToNotification) ?? [];apps/portal/src/app/views/notifications/notifications.component.ts (1)
466-468: Improve date handling to avoid timezone issuesThe current date handling might lead to inconsistencies across different timezones. Consider using UTC dates for consistency.
- value: new Date( - new Date(this.selectedFromDate).setDate(this.selectedFromDate.getDate()), - ).toISOString(), + value: new Date( + Date.UTC( + this.selectedFromDate.getFullYear(), + this.selectedFromDate.getMonth(), + this.selectedFromDate.getDate() + ) + ).toISOString(), // Similar change for selectedToDate - value: new Date( - new Date(this.selectedToDate).setDate(this.selectedToDate.getDate() + 1), - ).toISOString(), + value: new Date( + Date.UTC( + this.selectedToDate.getFullYear(), + this.selectedToDate.getMonth(), + this.selectedToDate.getDate() + 1 + ) + ).toISOString(),Also applies to: 476-478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/portal/src/app/graphql/graphql.queries.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.html(2 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(8 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 (5)
apps/portal/src/app/views/notifications/notifications.component.html (2)
55-57: Verify error handling in loadProvidersAndNotificationsForSelectedApplication.
The change to load both providers and notifications when an application is selected looks good. However, we should ensure proper error handling is in place:
- What happens if provider loading fails?
- Should notifications still load if provider loading fails?
✅ Verification successful
Let me gather more information about the implementation of this method.
Let me check the complete implementation of this component to understand the error handling better.
Error handling is properly implemented in loadProvidersAndNotificationsForSelectedApplication
The implementation has comprehensive error handling:
- Uses
catchErroroperator to handle API errors - Shows user-friendly error messages via p-toast
- Handles unauthorized access by redirecting to login
- Gracefully handles both provider and notification loading failures
- Resets relevant data (providers, notifications) when errors occur
- Properly manages loading state in all scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in the new method
ast-grep --pattern 'loadProvidersAndNotificationsForSelectedApplication($$$) {
try {
$$$
} catch($$$) {
$$$
}
}'
# Check for error message displays
rg -A 2 'p-toast' apps/portal/src/app/views/notifications/
Length of output: 450
Script:
#!/bin/bash
# Search for the method implementation
ast-grep --pattern 'loadProvidersAndNotificationsForSelectedApplication($$$) {
$$$
}'
# Search for error handling in service calls
rg -A 5 'getProviders|getNotifications.*subscribe' apps/portal/src/app/views/notifications/
Length of output: 830
Script:
#!/bin/bash
# Get the full component implementation
cat apps/portal/src/app/views/notifications/notifications.component.ts
# Check if there are any other error handlers
rg -A 3 'messageService.add' apps/portal/src/app/views/notifications/notifications.component.ts
Length of output: 16350
9-11: Verify provider dropdown integration with notification filtering.
The provider dropdown replacement looks good, but we should ensure:
- The providers list is properly populated when an application is selected
- The notification filtering correctly considers the selected provider
✅ Verification successful
Based on the code analysis, I can now provide a final response since I have sufficient evidence about the provider dropdown integration:
Provider dropdown is correctly integrated with notification filtering
The implementation shows proper integration:
- The
loadProvidersAndNotificationsForSelectedApplicationmethod correctly populates the providers list when an application is selected - The
getCombinedNotificationFiltersmethod properly includes the selected provider in the filter criteria:
if (this.selectedProvider) {
combinedNotificationFilters.push({
field: 'providerId',
operator: 'eq',
value: this.selectedProvider.toString(),
});
}- The provider filter is applied to both archived and non-archived notifications via the
loadNotificationsLazymethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for provider list population and filtering logic
ast-grep --pattern 'loadProvidersAndNotificationsForSelectedApplication($$$) {
$$$
}'
# Verify notification filtering considers provider
ast-grep --pattern 'loadNotificationsLazy($$$) {
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Let's try a different approach to find the component's TypeScript file and analyze the logic
fd "notifications.component.ts" --exec cat {}
# Also search for any provider-related service methods
rg "provider" -A 5 -B 5
Length of output: 76733
apps/portal/src/app/views/providers/provider.model.ts (1)
13-23: LGTM! Well-structured response interface
The ProviderAndNotificationResponse interface effectively combines providers and notifications with proper pagination fields and error handling.
apps/portal/src/app/graphql/graphql.queries.ts (1)
156-213: Verify the impact of archived notifications on performance
The query fetches both providers and archived notifications in a single request. Consider the performance implications when dealing with large archived datasets.
apps/portal/src/app/views/notifications/notifications.component.ts (1)
323-323: Add null check for channelType mapping
The code assumes that the provider's channelType will always exist in channelTypeMap.
| getProvidersAndNotifications( | ||
| variables, | ||
| inputToken, | ||
| archivedNotificationToggle, | ||
| ): Observable<ProviderAndNotificationResponse> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type definitions for method parameters
The parameters lack type definitions which could lead to runtime errors:
getProvidersAndNotifications(
- variables,
- inputToken,
- archivedNotificationToggle,
+ variables: {
+ providerLimit: number;
+ providerOffset: number;
+ providerFilters?: UniversalFilter[];
+ notificationLimit: number;
+ notificationOffset: number;
+ notificationFilters?: UniversalFilter[];
+ },
+ inputToken: string,
+ archivedNotificationToggle: boolean,
): Observable<ProviderAndNotificationResponse>📝 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.
| getProvidersAndNotifications( | |
| variables, | |
| inputToken, | |
| archivedNotificationToggle, | |
| ): Observable<ProviderAndNotificationResponse> { | |
| getProvidersAndNotifications( | |
| variables: { | |
| providerLimit: number; | |
| providerOffset: number; | |
| providerFilters?: UniversalFilter[]; | |
| notificationLimit: number; | |
| notificationOffset: number; | |
| notificationFilters?: UniversalFilter[]; | |
| }, | |
| inputToken: string, | |
| archivedNotificationToggle: boolean, | |
| ): Observable<ProviderAndNotificationResponse> { |
There was a problem hiding this comment.
fixed 1c14495
There was a problem hiding this comment.
@kshitij-k-osmosys Thanks for adding the type definitions! The use of unknown for variables is actually a better approach than my suggestion as it provides both type safety and flexibility. Great work! 🎉
(_/)
(^.^)
(")_(") ~♪ Happy to help!
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# View the commit changes
git show 1c14495549220073de170e90c19d0f63211f425d
Length of output: 2702
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| // Now that applications are loaded, load providers and notifications | ||
| this.loadProvidersAndNotificationsForSelectedApplication({ first: 0, rows: this.pageSize }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear providers and notifications on error
When there's an error loading applications, we should also clear the providers and notifications arrays to maintain consistency.
} else {
this.applications = [];
this.selectedApplication = null;
+ this.providers = [];
+ this.notifications = [];
+ this.totalRecords = 0;
}📝 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.
| // Now that applications are loaded, load providers and notifications | |
| this.loadProvidersAndNotificationsForSelectedApplication({ first: 0, rows: this.pageSize }); | |
| }); | |
| } | |
| // Now that applications are loaded, load providers and notifications | |
| this.loadProvidersAndNotificationsForSelectedApplication({ first: 0, rows: this.pageSize }); | |
| } else { | |
| this.applications = []; | |
| this.selectedApplication = null; | |
| this.providers = []; | |
| this.notifications = []; | |
| this.totalRecords = 0; | |
| } | |
| } |
There was a problem hiding this comment.
This is not a part of if else block
There was a problem hiding this comment.
@kshitij-k-osmosys You're absolutely right - I apologize for the confusion. The loadProvidersAndNotificationsForSelectedApplication call is not part of the if-else block but rather a separate step that happens after the application loading logic. I'm withdrawing my suggestion as it was incorrect. Thank you for the clarification!
| } else if ( | ||
| providerAndNotificationResponse?.providers?.length && | ||
| providerAndNotificationResponse?.notifications?.length | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate provider and notification handling
The current condition requires both providers AND notifications to be present. This might prevent showing available providers when there are no notifications.
- } else if (
- providerAndNotificationResponse?.providers?.length &&
- providerAndNotificationResponse?.notifications?.length
- ) {
+ } else if (providerAndNotificationResponse?.providers?.length) {
// Fetch list of providers for dropdown
this.providers = providerAndNotificationResponse.providers.map((obj) => ({
label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`,
value: obj.providerId,
}));
+
+ if (providerAndNotificationResponse?.notifications?.length) {
this.notifications = providerAndNotificationResponse.notifications;
this.totalRecords = providerAndNotificationResponse.notificationTotal;
+ } else {
+ this.notifications = [];
+ this.totalRecords = 0;
+ }📝 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.
| } else if ( | |
| providerAndNotificationResponse?.providers?.length && | |
| providerAndNotificationResponse?.notifications?.length | |
| ) { | |
| } else if (providerAndNotificationResponse?.providers?.length) { | |
| // Fetch list of providers for dropdown | |
| this.providers = providerAndNotificationResponse.providers.map((obj) => ({ | |
| label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`, | |
| value: obj.providerId, | |
| })); | |
| if (providerAndNotificationResponse?.notifications?.length) { | |
| this.notifications = providerAndNotificationResponse.notifications; | |
| this.totalRecords = providerAndNotificationResponse.notificationTotal; | |
| } else { | |
| this.notifications = []; | |
| this.totalRecords = 0; | |
| } |
There was a problem hiding this comment.
fixed 3fe2a09
There was a problem hiding this comment.
@kshitij-k-osmosys Thank you for implementing the fix! The separation of provider and notification handling will make the code more robust.
🔗 Analysis chain
@kshitij-k-osmosys Thank you for implementing the fix! The separation of provider and notification handling will make the code more robust.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the specific commit details
git show 3fe2a0911f6727b1a448be7ed834656d6ee29694
Length of output: 1902
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
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-39: Add token validation before using it in the GraphQL query.The service has a
decodeTokenmethod, but it's not being utilized to validate the token before making the GraphQL query. Consider adding validation to prevent potential issues with malformed tokens.getApplications(variables: unknown, inputToken: string): Observable<ApplicationResponse> { + try { + // Validate token format before using + this.decodeToken(inputToken); + } catch (error) { + return new Observable((subscriber) => { + subscriber.error(new Error('Invalid token format')); + }); + } return this.graphqlService.query(GetApplications, variables, inputToken).pipe(
🧹 Nitpick comments (5)
apps/portal/src/app/views/applications/applications.service.ts (2)
22-22: Consider using a specific interface for variables instead ofunknown.While using
unknownis safer thanany, GraphQL queries typically expect specific shapes for their variables. Consider creating and using a dedicated interface to provide better type safety and developer experience.interface GetApplicationsVariables { offset?: number; limit?: number; // Add other expected variables } getApplications(variables: GetApplicationsVariables, inputToken: string)
Line range hint
22-39: Consider architectural separation between application and provider concerns.Based on the PR objectives, a new provider service is being introduced. To maintain clear separation of concerns:
- This service should focus solely on application-related operations.
- Provider-related operations should be handled by the new provider service.
- Consider using a facade service if tight coupling between applications and providers is needed.
apps/portal/src/app/views/providers/providers.service.ts (1)
48-119: Consider refactoring duplicated error handling and complex mapping logic.
- The error handling logic is duplicated between the archived and non-archived branches.
- The archived notification mapping logic is complex and could be extracted into a helper method.
export class ProvidersService { + private handleError(error: Error): Observable<never> { + const errorMessage: string = error.message; + throw new Error(errorMessage); + } + + private mapArchivedNotificationToNotification(item: ArchivedNotification): Notification { + return { + id: item.notificationId, + channelType: item.channelType, + data: item.data, + deliveryStatus: item.deliveryStatus, + result: item.result, + createdOn: item.createdOn, + updatedOn: item.updatedOn, + createdBy: item.createdBy, + updatedBy: item.updatedBy, + status: item.status, + }; + } getProvidersAndNotifications( variables: unknown, inputToken: string, archivedNotificationToggle: boolean, ): Observable<ProviderAndNotificationResponse> { if (archivedNotificationToggle) { return this.graphqlService .query(GetProvidersAndArchivedNotifications, variables, inputToken) .pipe( map((response: ApolloQueryResult<GetProviderArchivedNotificationResponse>) => { const providerArray = response.data?.providers.providers; const archivedNotificationArray = response.data?.archivedNotifications.archivedNotifications; - const convertToNotificationArray = - archivedNotificationArray?.map((item) => ({ - id: item.notificationId, - channelType: item.channelType, - data: item.data, - deliveryStatus: item.deliveryStatus, - result: item.result, - createdOn: item.createdOn, - updatedOn: item.updatedOn, - createdBy: item.createdBy, - updatedBy: item.updatedBy, - status: item.status, - })) ?? []; + const convertToNotificationArray = + archivedNotificationArray?.map(this.mapArchivedNotificationToNotification) ?? []; const providerArchivedResponseObject: ProviderAndNotificationResponse = { providers: providerArray, providerTotal: response.data?.providers.total, providerOffset: response.data?.providers.offset, providerLimit: response.data?.providers.limit, notifications: convertToNotificationArray, notificationTotal: response.data?.archivedNotifications.total, notificationOffset: response.data?.archivedNotifications.offset, notificationLimit: response.data?.archivedNotifications.limit, errors: response.errors || null, }; return providerArchivedResponseObject; }), - catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); - }), + catchError(this.handleError), ); } return this.graphqlService.query(GetProvidersAndNotifications, variables, inputToken).pipe( map((response: ApolloQueryResult<GetProviderNotificationResponse>) => { const providerArray = response.data?.providers.providers; const notificationArray = response.data?.notifications.notifications; const providerNotificationResponseObject: ProviderAndNotificationResponse = { providers: providerArray, providerTotal: response.data?.providers.total, providerOffset: response.data?.providers.offset, providerLimit: response.data?.providers.limit, notifications: notificationArray, notificationTotal: response.data?.notifications.total, notificationOffset: response.data?.notifications.offset, notificationLimit: response.data?.notifications.limit, errors: response.errors || null, }; return providerNotificationResponseObject; }), - catchError((error) => { - const errorMessage: string = error.message; - throw new Error(errorMessage); - }), + catchError(this.handleError), ); } }apps/portal/src/app/views/notifications/notifications.component.ts (2)
328-334: Improve provider label construction with proper null checks.The current implementation could be improved to handle missing or invalid channel types more gracefully.
- label: this.channelTypeMap[obj.channelType]?.altText - ? `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}` - : `${obj.name} - ${this.channelTypeMap[ChannelType.UNKNOWN].altText}`, + label: obj.name + (obj.channelType && this.channelTypeMap[obj.channelType] + ? ` - ${this.channelTypeMap[obj.channelType].altText}` + : ` - ${this.channelTypeMap[ChannelType.UNKNOWN].altText}`), value: obj.providerId,
482-484: Simplify date manipulation in filter construction.The date manipulation logic could be simplified using a utility function to avoid repetition and improve readability.
+ private adjustDateForFilter(date: Date, addDays: number = 0): string { + const adjustedDate = new Date(date); + adjustedDate.setDate(date.getDate() + addDays); + return adjustedDate.toISOString(); + } if (this.selectedFromDate) { combinedNotificationFilters.push({ field: 'createdOn', operator: 'gt', - value: new Date( - new Date(this.selectedFromDate).setDate(this.selectedFromDate.getDate()), - ).toISOString(), + value: this.adjustDateForFilter(this.selectedFromDate), }); } if (this.selectedToDate) { combinedNotificationFilters.push({ field: 'createdOn', operator: 'lt', - value: new Date( - new Date(this.selectedToDate).setDate(this.selectedToDate.getDate() + 1), - ).toISOString(), + value: this.adjustDateForFilter(this.selectedToDate, 1), }); }Also applies to: 493-494
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/portal/src/app/views/applications/applications.service.ts(1 hunks)apps/portal/src/app/views/notifications/notifications.component.ts(8 hunks)apps/portal/src/app/views/notifications/notifications.service.ts(1 hunks)apps/portal/src/app/views/providers/providers.service.ts(1 hunks)
🔇 Additional comments (6)
apps/portal/src/app/views/applications/applications.service.ts (1)
22-22: Verify all callers are updated for the new parameter types.The method signature change from untyped parameters to
variables: unknown, inputToken: stringmight affect existing callers.✅ Verification successful
Method signature change appears compatible with existing usage
The codebase analysis shows:
- The only usage in notifications.component.ts is type-compatible
- No TypeScript compilation errors were found
- No other callers or declarations exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to getApplications to ensure they're updated rg "getApplications\(" --type ts # Look for potential compilation errors in TypeScript files # that might indicate incompatible calls rg "TS2345|TS2322" --type tsLength of output: 334
Script:
#!/bin/bash # Get context around the usage in notifications component rg "getApplications\(" -B 5 -A 5 apps/portal/src/app/views/notifications/notifications.component.ts # Look for any interfaces or abstract classes that might declare this method ast-grep --pattern 'interface $_ { $$$ getApplications($_) $$$ }' ast-grep --pattern 'abstract class $_ { $$$ getApplications($_) $$$ }'Length of output: 534
Script:
#!/bin/bash # Search for variables and loginToken declarations in the notifications component ast-grep --pattern 'variables = $_' --lang ts apps/portal/src/app/views/notifications/notifications.component.ts ast-grep --pattern 'loginToken = $_' --lang ts apps/portal/src/app/views/notifications/notifications.component.ts # Get a broader context of the component file to understand variable declarations rg "class NotificationsComponent" -A 20 apps/portal/src/app/views/notifications/notifications.component.tsLength of output: 1080
apps/portal/src/app/views/notifications/notifications.service.ts (1)
32-32: LGTM! Type safety improvement.The explicit type annotations enhance type safety and code clarity.
apps/portal/src/app/views/providers/providers.service.ts (1)
12-40: LGTM! Well-structured interfaces.The interfaces are well-defined with appropriate optional properties, providing clear type definitions for the GraphQL responses.
apps/portal/src/app/views/notifications/notifications.component.ts (3)
10-11: LGTM! Clean property and import declarations.The new properties and imports are well-organized and properly typed.
Also applies to: 48-48, 54-54
274-277: Consider implementing logout for missing token.When the login token is missing, consider implementing a logout mechanism to maintain consistent application state.
Would you like me to help create a new GitHub issue to track the implementation of the logout functionality?
459-507: LGTM! Well-structured filter construction.The
getCombinedNotificationFiltersmethod is well-organized and handles all filter cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/portal/src/app/views/notifications/notifications.component.ts (4)
48-48: Add type annotation for providers array.For better type safety and code clarity, specify the type for the providers array.
- providers = []; + providers: Array<{ label: string; value: string }> = [];
326-330: Improve channel type mapping robustness.The current implementation could be more robust by using nullish coalescing and optional chaining.
- label: this.channelTypeMap[obj.channelType]?.altText - ? `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}` - : `${obj.name} - ${this.channelTypeMap[ChannelType.UNKNOWN].altText}`, + label: `${obj.name} - ${this.channelTypeMap[obj.channelType]?.altText ?? this.channelTypeMap[ChannelType.UNKNOWN].altText}`, value: obj.providerId,
479-482: Simplify date handling logic.The date manipulation logic is complex and could be simplified using Date methods directly.
- value: new Date( - new Date(this.selectedFromDate).setDate(this.selectedFromDate.getDate()), - ).toISOString(), + value: this.selectedFromDate.toISOString(), - value: new Date( - new Date(this.selectedToDate).setDate(this.selectedToDate.getDate() + 1), - ).toISOString(), + value: new Date(this.selectedToDate.setHours(23, 59, 59, 999)).toISOString(),Also applies to: 489-492
456-504: Consider extracting filter logic to a service.The filter logic in
getCombinedNotificationFilterscould be moved to a dedicated service to improve maintainability and reusability.This would:
- Make the component more focused on presentation logic
- Allow reuse of filter logic across different components
- Make testing easier by isolating the filter logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/app/views/notifications/notifications.component.ts(8 hunks)
🔇 Additional comments (2)
apps/portal/src/app/views/notifications/notifications.component.ts (2)
87-93: LGTM! Clean dependency injection and initialization.The constructor properly injects the ProvidersService, and ngOnInit correctly initializes the component.
178-180: Verify application selection before loading providers.There's a potential race condition where providers and notifications are loaded before selectedApplication is set. Consider moving this call inside the if block where applications are loaded successfully.
if (this.applications.length > 0) { this.selectedApplication = this.applications[0].value; + // Now that applications are loaded and selected, load providers and notifications + this.loadProvidersAndNotificationsForSelectedApplication({ first: 0, rows: this.pageSize }); } else { this.selectedApplication = null; } - // Now that applications are loaded, load providers and notifications - this.loadProvidersAndNotificationsForSelectedApplication({ first: 0, rows: this.pageSize });
| if (!loginToken) { | ||
| // Handle missing token | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for missing login token.
The error handling for missing login token should follow the same pattern as other error cases in the component.
if (!loginToken) {
- // Handle missing token
+ this.messageService.add({
+ key: 'tst',
+ severity: 'error',
+ summary: 'Error',
+ detail: 'Login token is missing. Please log in again.',
+ });
+ this.loading = false;
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!loginToken) { | |
| // Handle missing token | |
| return; | |
| } | |
| if (!loginToken) { | |
| this.messageService.add({ | |
| key: 'tst', | |
| severity: 'error', | |
| summary: 'Error', | |
| detail: 'Login token is missing. Please log in again.', | |
| }); | |
| this.loading = false; | |
| return; | |
| } |
There was a problem hiding this comment.
Handled in different PR
We log out in this scenario
https://github.com/OsmosysSoftware/osmo-x/pull/373/files#r1887016153
There was a problem hiding this comment.
@kshitij-k-osmosys Understood. Thanks for clarifying that the missing login token scenario is handled by logging out the user.
🐰
╱|、
(˚ˎ 。7
|、˜〵
じしˍ,)ノ
*thinking*
✏️ Learnings added
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#373
File: apps/portal/src/app/views/notifications/notifications.component.ts:271-274
Timestamp: 2025-01-17T08:16:12.033Z
Learning: In the OsmoX portal application, when a login token is missing, the user should be logged out instead of showing an error message.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Portal PR Checklist
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:
Fetch provider list on portal based on selected application
Related changes:
providergetProvidersForSelectedApplication()and call it on initial loadScreenshots:
Screencast.from.2024-12-16.17-26-58.mp4
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores