feat: channel updates and fixes. #678
Conversation
marvinIsSacul
added
enhancement
microfrontend-poc
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several enhancements across multiple components in the application. Key changes include the addition of interactive buttons in alert dialogs, improved error handling and validation in channel screens, and modifications to the routing configuration. Additionally, several files related to the sidebar application, including configurations and components, have been removed, indicating a significant restructuring of that part of the application. Changes
Possibly related PRs
Poem
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 (
|
| showAlert( | ||
| 'Error creating a new channel. ' + error?.response?.data, | ||
| 'Channel must have exactly only 1 primary route.', |
There was a problem hiding this comment.
Shouldn't we handle this on the add route modal?
There was a problem hiding this comment.
It's only at this component whereby we have all the information that we need about all of the channels and their respective routes.
At the deeper nested components, we only have access to the 1 individual route that we're modifying and/or adding.
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
packages/dashboard-app/src/components/ux/error.component.tsx (1)
13-20: Consider using a responsive height for the component.The component uses a fixed height of
300px, which may not be responsive on different screen sizes. Consider using a more flexible height value or a minimum height to ensure the component adapts to various screen sizes.Apply this diff to use a minimum height:
<Box display="flex" flexDirection="column" justifyContent="center" alignItems="center" - height="300px" + minHeight="300px" padding={2} >
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/sidebar-app/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (23)
- packages/channels-app/src/components/dialogs/alert.dialog.component.tsx (2 hunks)
- packages/channels-app/src/screens/add.channel.screen.tsx (3 hunks)
- packages/channels-app/src/screens/edit.channel.screen.tsx (2 hunks)
- packages/channels-app/src/screens/manage.channels.screen.tsx (2 hunks)
- packages/channels-app/src/screens/steps/RequestMatching.tsx (1 hunks)
- packages/channels-app/src/screens/steps/routes/ChannelRoutes.tsx (2 hunks)
- packages/dashboard-app/src/components/charts.component.tsx (5 hunks)
- packages/dashboard-app/src/components/ux/error.component.tsx (1 hunks)
- packages/rbac-app/src/components/dialogs/alert.dialog.component.tsx (2 hunks)
- packages/root-config/src/microfrontend-layout.html (1 hunks)
- packages/sidebar-app/.eslintrc (0 hunks)
- packages/sidebar-app/.gitignore (0 hunks)
- packages/sidebar-app/.prettierignore (0 hunks)
- packages/sidebar-app/babel.config.json (0 hunks)
- packages/sidebar-app/jest.config.js (0 hunks)
- packages/sidebar-app/package.json (0 hunks)
- packages/sidebar-app/src/declarations.d.ts (0 hunks)
- packages/sidebar-app/src/jembi-openhim-sidebar.tsx (0 hunks)
- packages/sidebar-app/src/menu.component.tsx (0 hunks)
- packages/sidebar-app/src/root.component.tsx (0 hunks)
- packages/sidebar-app/tsconfig.json (0 hunks)
- packages/sidebar-app/webpack.config.js (0 hunks)
- packages/users-app/src/components/dialogs/alert.dialog.component.tsx (2 hunks)
Files not reviewed due to no reviewable changes (12)
- packages/sidebar-app/.eslintrc
- packages/sidebar-app/.gitignore
- packages/sidebar-app/.prettierignore
- packages/sidebar-app/babel.config.json
- packages/sidebar-app/jest.config.js
- packages/sidebar-app/package.json
- packages/sidebar-app/src/declarations.d.ts
- packages/sidebar-app/src/jembi-openhim-sidebar.tsx
- packages/sidebar-app/src/menu.component.tsx
- packages/sidebar-app/src/root.component.tsx
- packages/sidebar-app/tsconfig.json
- packages/sidebar-app/webpack.config.js
Additional comments not posted (17)
packages/channels-app/src/components/dialogs/alert.dialog.component.tsx (1)
23-36: LGTM!The changes enhance the functionality of the
AlertDialogcomponent by providing a clear action to acknowledge the alert. The button style and color are appropriately configured based on the severity of the alert, and it correctly triggers theonClosefunction to dismiss the alert dialog. The changes improve the user experience by providing a more interactive way to dismiss the alert dialog.packages/rbac-app/src/components/dialogs/alert.dialog.component.tsx (1)
23-36: LGTM!The addition of the action button within the
Alertcomponent is a great enhancement to the user experience. It provides a clear and accessible way for users to acknowledge and dismiss the alert.The code follows best practices by:
- Utilizing the
Buttoncomponent from Material-UI for consistent styling and behavior.- Dynamically setting the button's color based on the
severityprop, ensuring visual consistency with the alert's severity.- Correctly linking the button's
onClickevent to theonCloseprop of theAlertDialog, allowing the alert to be dismissed when clicked.Overall, the changes are well-implemented and improve the usability of the
AlertDialogcomponent.packages/users-app/src/components/dialogs/alert.dialog.component.tsx (1)
23-36: LGTM!The addition of the
actionprop to theAlertcomponent is a great enhancement to the user experience. It provides a clear and intuitive way for users to acknowledge and dismiss the alert.Some key observations:
- Setting the
colorof the button based on theseverityprop effectively communicates the nature of the alert to the user.- The
onClickhandler being set to theonCloseprop ensures that the alert is dismissed when the button is clicked.- The button text "OK" is a standard and intuitive label for acknowledging an alert.
Overall, these changes improve the usability and clarity of the
AlertDialogcomponent.packages/root-config/src/microfrontend-layout.html (1)
80-80: LGTM!The removal of the
exactattribute from the route configuration for channels is a good change. It allows for more flexible navigation within the channels section of the application by matching any URL that starts with#!/channels.This change is consistent with the removal of the earlier instance of the
#!/channelsroute and simplifies the routing logic.packages/dashboard-app/src/components/charts.component.tsx (4)
11-11: LGTM!The import statement for the
ErrorMessagecomponent is correctly written.
24-24: LGTM!The
errorstate variable is correctly declared and initialized with the appropriate type annotation.
40-40: LGTM!Resetting the
errorstate tonullbefore making the API call is a good practice to ensure that any previous error state is cleared.
51-51: LGTM!The error handling logic is implemented correctly:
- The
errorstate is set when an error occurs during data fetching.- The
ErrorMessagecomponent is rendered when an error exists, providing a clear indication to the user.- The
getFilteredTransactionsfunction is passed as theonRetryprop, allowing the user to retry fetching the transactions if an error occurs.Also applies to: 68-71
packages/channels-app/src/screens/edit.channel.screen.tsx (2)
47-58: LGTM!The validation check to ensure that a channel has exactly one primary route is a good addition. It prevents invalid channel configurations and maintains data integrity. The code correctly filters the routes, checks the count of primary routes, and displays an informative alert message if the validation fails. The early return also prevents the mutation from being executed in case of validation failure.
88-88: Looks good!The minor adjustment to the width of the
Papercomponent from600pxto680pxis acceptable. It provides more space for the content within the component and does not affect its functionality.packages/channels-app/src/screens/add.channel.screen.tsx (3)
74-78: LGTM!The improved error handling provides a more informative error message to the user, enhancing the user experience.
81-92: The past review comment is still valid.The comment by drizzentic on line 88 suggests handling this validation on the add route modal. Please consider addressing this suggestion.
132-132: LGTM!The increased width of the
Papercomponent may improve the visual presentation of the screen.packages/channels-app/src/screens/steps/routes/ChannelRoutes.tsx (1)
159-170: LGTM!The code changes enhance the user interface by clearly distinguishing primary routes from others using a "primary" chip. The Tooltip component improves usability by providing additional context about the "primary" label. The implementation is correct and follows best practices.
packages/channels-app/src/screens/manage.channels.screen.tsx (1)
Line range hint
92-117: Verify thatselectedChannelis always set correctly.The function logic for disabling or enabling a channel based on its current status is correct. However, the function now relies on the
selectedChannelstate variable to determine the channel to modify.Ensure that
selectedChannelis always set correctly before invokingonActionDisableChannel. You can run the following script to verify this:Also applies to: 183-183
Verification successful
Add a null check for
selectedChannelinonActionDisableChannelThe usage of
selectedChannelappears to be correct within the context menu scenario. However, to improve robustness and prevent potential errors, consider adding a null check at the beginning of theonActionDisableChannelfunction:const onActionDisableChannel = () => { if (!selectedChannel) return; // Add this line const channel = selectedChannel; // ... rest of the function }This change will ensure that the function gracefully handles cases where
selectedChannelmight be null.Additionally, verify that the context menu can only be opened when a channel is selected, which should be the case based on the current implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `selectedChannel` is always set correctly before invoking `onActionDisableChannel`. # Test: Search for invocations of `onActionDisableChannel`. Expect: `selectedChannel` is always set before invoking `onActionDisableChannel`. rg --type typescript -A 5 $'onActionDisableChannel'Length of output: 85
Script:
#!/bin/bash # Description: Search for `onActionDisableChannel` and `selectedChannel` in TypeScript files # Find TypeScript files and search for `onActionDisableChannel` echo "Searching for onActionDisableChannel:" fd -e ts -e tsx | xargs grep -n -A 5 "onActionDisableChannel" echo -e "\nSearching for selectedChannel:" # Find TypeScript files and search for `selectedChannel` fd -e ts -e tsx | xargs grep -n -B 5 -A 5 "selectedChannel"Length of output: 4968
packages/channels-app/src/screens/steps/RequestMatching.tsx (2)
65-70: LGTM!The conditional rendering of the start and end adornments based on
channel.addAutoRewriteRulesis implemented correctly. The adornments provide clear visual cues to the user about the automatic addition of regex delimiters when the feature is enabled.
72-79: LGTM!The modification of the
urlPatterninput value based onchannel.addAutoRewriteRulesis implemented correctly. Trimming theurlPatternand removing the leading^and trailing$characters whenaddAutoRewriteRulesis enabled ensures a cleaner input format and aligns with the visual cues provided by the adornments. Using the originalurlPatternwhenaddAutoRewriteRulesis disabled maintains the expected behavior for manual regex input.
| height="300px" | ||
| padding={2} | ||
| > | ||
| <ErrorOutlineIcon color="error" sx={{fontSize: 60}} /> |
There was a problem hiding this comment.
Add accessibility attributes to the error icon.
The error icon does not have an aria-label or alt text, which can make it difficult for screen reader users to understand its purpose.
Apply this diff to add an aria-label:
-<ErrorOutlineIcon color="error" sx={{fontSize: 60}} />
+<ErrorOutlineIcon color="error" sx={{fontSize: 60}} aria-label="Error icon" />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.
| <ErrorOutlineIcon color="error" sx={{fontSize: 60}} /> | |
| <ErrorOutlineIcon color="error" sx={{fontSize: 60}} aria-label="Error icon" /> |
| <Typography variant="h6" color="error" align="center" gutterBottom> | ||
| Oops! Something went wrong. | ||
| </Typography> | ||
| <Typography variant="body1" align="center" gutterBottom> | ||
| An error occurred while loading the data. Please try again. | ||
| </Typography> |
There was a problem hiding this comment.
Use the title and message props for customization.
The title and message props are not being utilized in the component. Instead, hardcoded values are being rendered. To make the component more reusable and customizable, consider using the prop values.
Apply this diff to use the prop values:
-<Typography variant="h6" color="error" align="center" gutterBottom>
- Oops! Something went wrong.
+<Typography variant="h6" color="error" align="center" gutterBottom>
+ {title || 'Oops! Something went wrong.'}
</Typography>
<Typography variant="body1" align="center" gutterBottom>
- An error occurred while loading the data. Please try again.
+ {message || 'An error occurred while loading the data. Please try again.'}
</Typography>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.
| <Typography variant="h6" color="error" align="center" gutterBottom> | |
| Oops! Something went wrong. | |
| </Typography> | |
| <Typography variant="body1" align="center" gutterBottom> | |
| An error occurred while loading the data. Please try again. | |
| </Typography> | |
| <Typography variant="h6" color="error" align="center" gutterBottom> | |
| {title || 'Oops! Something went wrong.'} | |
| </Typography> | |
| <Typography variant="body1" align="center" gutterBottom> | |
| {message || 'An error occurred while loading the data. Please try again.'} | |
| </Typography> |
| <Button variant="contained" color="primary" onClick={onRetry}> | ||
| Retry | ||
| </Button> |
There was a problem hiding this comment.
Improve the retry button's accessibility.
The retry button does not have an aria-label, which can make it less descriptive for users relying on assistive technologies. Adding an aria-label can provide more context about the button's action.
Apply this diff to add an aria-label:
-<Button variant="contained" color="primary" onClick={onRetry}>
+<Button variant="contained" color="primary" onClick={onRetry} aria-label="Retry loading data">
Retry
</Button>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.
| <Button variant="contained" color="primary" onClick={onRetry}> | |
| Retry | |
| </Button> | |
| <Button variant="contained" color="primary" onClick={onRetry} aria-label="Retry loading data"> | |
| Retry | |
| </Button> |
| @@ -71,20 +68,30 @@ function AddChannelScreen() { | |||
| }, | |||
| onSuccess: () => { | |||
| hideBackdrop() | |||
| navigate(Routes.MANAGE_CHANNELS) | |||
| window.location.href = `/#${Routes.MANAGE_CHANNELS}` | |||
There was a problem hiding this comment.
Use React Router's navigate function for navigation.
Directly setting window.location.href bypasses React Router's navigation capabilities and may impact the user experience. Consider using the navigate function from React Router for a more seamless navigation experience.
- window.location.href = `/#${Routes.MANAGE_CHANNELS}`
+ navigate(`${Routes.MANAGE_CHANNELS}`)Committable suggestion was skipped due to low confidence.
| @@ -170,7 +177,7 @@ function AddChannelScreen() { | |||
| <Button | |||
| variant="outlined" | |||
| color="primary" | |||
| onClick={() => navigate(-1)} | |||
| href={`/#${Routes.MANAGE_CHANNELS}`} | |||
There was a problem hiding this comment.
Use React Router's navigate function for navigation.
Directly setting the href attribute bypasses React Router's navigation capabilities and may impact the user experience. Consider using the navigate function from React Router for a more seamless navigation experience.
- href={`/#${Routes.MANAGE_CHANNELS}`}
+ onClick={() => navigate(`${Routes.MANAGE_CHANNELS}`)}Committable suggestion was skipped due to low confidence.
…-console into feature/CI-86c082m9k_channels-fixes
Summary by CodeRabbit
Release Notes
New Features
AlertDialogfor easier dismissal of alerts.Chartscomponent with a retry option.RequestMatching.Improvements
AddChannelScreenandEditChannelScreenfor primary routes.AddChannelScreento enhance user experience.ChannelRoutes.Bug Fixes
Chores
sidebar-app, which may affect project setup.