feat: Edit infrastructure monitor page #1308
Conversation
peterpardo
changed the title
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
Large PR Notification
Dear contributor,
Thank you for your substantial contribution to this project. LlamaPReview has detected that this Pull Request contains a large volume of changes, which exceeds our current processing capacity.
Details:
- PR and related contents total size: Approximately 51,530 characters
- Current limit: 50,000 characters
Next steps:
- Consider breaking this PR into smaller, more focused changes if possible.
- For manual review, please reach out to your team members or maintainers.
We appreciate your understanding and commitment to improving this project. Your contributions are valuable, and we want to ensure they receive the attention they deserve.
LlamaPReview is continuously evolving to better serve the community. Share your thoughts on handling large PRs in our GitHub Discussions - your feedback helps us improve and expand our capabilities.
If you have any questions or need assistance, our community and support team are here to help.
Best regards,
LlamaPReview Team
WalkthroughThe pull request introduces a new route for configuring infrastructure monitors in a React application, enhancing the routing functionality by allowing access to a dedicated configuration page. Modifications to state management are made in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
Client/src/Pages/Infrastructure/Configure/index.jsx (5)
347-347: Simplify conditional expression inerrorpropThe expression
errors["url"] ? true : falseis unnecessary. You can simplify it by directly using!!errors["url"]orBoolean(errors["url"]).Apply this diff to simplify the expression:
-error={errors["url"] ? true : false} +error={!!errors["url"]}🧰 Tools
🪛 Biome (1.9.4)
[error] 347-347: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
369-369: Simplify conditional expression inerrorpropThe expression
errors["secret"] ? true : falseis unnecessary. You can simplify it by directly using!!errors["secret"]orBoolean(errors["secret"]).Apply this diff to simplify the expression:
-error={errors["secret"] ? true : false} +error={!!errors["secret"]}🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
93-93: Use strict equality operatorsIn JavaScript, it's best practice to use strict equality (
===and!==) to avoid type coercion issues. In line 93, consider replacing==with===.Apply this diff:
-[id]: prev[id] == undefined && value == "on" ? true : !prev[id], +[id]: prev[id] === undefined && value === "on" ? true : !prev[id],
267-267: Correct typo in status messageThe word "Editting..." should be "Editing..." to fix the typo.
Apply this diff to correct the typo:
-Editting... +Editing...
170-170: Fix typo in TODO commentThere's a typo in the TODO comment. "functionalityf" should be "functionality".
Apply this diff:
-// TODO: add update infrastructure monitor functionalityf +// TODO: add update infrastructure monitor functionalityClient/src/Pages/Infrastructure/index.jsx (2)
3-3: Clean up import statementPlease remove the commented-out
useDispatchimport to tidy up the code.Apply this diff:
-import { /* useDispatch, */ useDispatch, useSelector } from "react-redux"; +import { useDispatch, useSelector } from "react-redux";
134-137: IncludedispatchinuseEffectdependency arrayTo ensure consistency and avoid potential issues, include
dispatchin the dependency array of theuseEffecthook.Apply this diff:
-useEffect(() => { +useEffect(() => { dispatch(resetInfrastructureMonitorFormAction()); -}, []); +}, [dispatch]);Client/src/Features/InfrastructureMonitors/infrastructureMonitorsSlice.js (1)
261-261: Review usage ofsuccessflag during pending statesSetting
successtofalseduring pending actions may be misleading. Consider leavingsuccessasnulluntil the action is fulfilled or rejected to accurately reflect the loading state.Also applies to: 281-281, 300-300, 320-320, 340-340, 361-361, 380-380, 399-399, 419-419
Client/src/Pages/Infrastructure/Details/index.jsx (2)
502-521: Mom's spaghetti! Extract tooltip into a reusable componentThe tooltip configuration with modifiers could be reused across the application.
Consider extracting into a shared component:
+ const StatusTooltip = ({ children, title }) => ( + <Tooltip + title={title} + disableInteractive + slotProps={{ + popper: { + modifiers: [ + { + name: "offset", + options: { + offset: [0, -8], + }, + }, + ], + }, + }} + > + {children} + </Tooltip> + );
Line range hint
607-619: Knees weak! Address the FE team comments and improve layout maintainabilityThe comments requesting "FE team HELP!" indicate potential layout issues that need attention.
Consider these improvements:
- Use CSS Grid instead of flexbox for the chart layout
- Create a constant for the layout calculations
- Document the layout constraints and requirements
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
Client/src/App.jsx(2 hunks)Client/src/Features/InfrastructureMonitors/infrastructureMonitorsSlice.js(13 hunks)Client/src/Pages/Infrastructure/Configure/index.jsx(1 hunks)Client/src/Pages/Infrastructure/Details/index.jsx(5 hunks)Client/src/Pages/Infrastructure/constants.js(1 hunks)Client/src/Pages/Infrastructure/index.jsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
Client/src/Pages/Infrastructure/Configure/index.jsx
[error] 347-347: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 369-369: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (2)
Client/src/Pages/Infrastructure/constants.js (1)
1-2: Constants are appropriately defined
The constants THRESHOLD_FIELD_PREFIX and HARDWARE_MONITOR_TYPES are correctly defined and exported.
Client/src/App.jsx (1)
143-146: New route for configuring infrastructure monitors added correctly
The route for ConfigureInfrastructureMonitor has been added properly, enabling navigation to the configuration page.
| {isAdmin && ( | ||
| <Button | ||
| variant="contained" | ||
| color="secondary" | ||
| onClick={() => navigate(`/infrastructure/configure/${monitorId}`)} | ||
| sx={{ | ||
| px: theme.spacing(5), | ||
| "& svg": { | ||
| mr: theme.spacing(3), | ||
| "& path": { | ||
| stroke: theme.palette.text.tertiary, | ||
| }, | ||
| }, | ||
| }} | ||
| > | ||
| <SettingsIcon /> Configure | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Arms are heavy! Extract route paths into constants
The hardcoded navigation path could cause maintenance issues if routes change.
Create a routes constant file:
+ // src/constants/routes.js
+ export const ROUTES = {
+ INFRASTRUCTURE_CONFIGURE: (id) => `/infrastructure/configure/${id}`,
+ };
// In your component:
- onClick={() => navigate(`/infrastructure/configure/${monitorId}`)}
+ onClick={() => navigate(ROUTES.INFRASTRUCTURE_CONFIGURE(monitorId))}Committable suggestion skipped: line range outside the PR's diff.
| @@ -182,7 +183,7 @@ GaugeBox.propTypes = { | |||
| * Renders the infrastructure details page | |||
| * @returns {React.ReactElement} Infrastructure details page component | |||
| */ | |||
| const InfrastructureDetails = () => { | |||
| const InfrastructureDetails = ({ isAdmin }) => { | |||
There was a problem hiding this comment.
Yo! Add PropTypes validation for isAdmin prop
The isAdmin prop needs PropTypes validation to prevent potential runtime issues.
Add this after the component:
+ InfrastructureDetails.propTypes = {
+ isAdmin: PropTypes.bool.isRequired
+ };Committable suggestion skipped: line range outside the PR's diff.
|
Hi @peterpardo ! As always we greatly appreciate your contributions to the project! We haven't decided on an approach for the "Configure" pages yet however, so we won't be able to address this PR at the current time. The issue at hand is that the "Configure" pages are practically identical to the "Create" pages, so it probably makes sense just to refactor the "Create" pages for dual use. Appreciate your hard work here, if we decide to go with the approach of having two separate pages we can revisit this PR! |
Sure! No worries. Actually, I noticed that as well that there are code duplications in some parts of the pages. I can try to make a PR to refactor the Create and Configure pages, but I'll implement it on the Infrastructure Monitor only first to avoid a large PR. Let me know if that sounds good. |
I think it would be best to hold off until we decide what direction we're going with these pages. The maintainers are meeting tomorrow so we'll discuss this topic then, I'll update the issues page after that with the results of our discussion. Thanks! |
Summary
This PR adds the "Configure" button at the infrastructure details page to edit the selected infrastructure.
Changes Made
Related Issue/s
Fixes #1277
Screenshots / Demo Video
checkmate-issue-1277-demo-1.mp4
Additional Context/s