1067 fe hardware monitoring create hardware monitor

[jennifer-gan]

• Created custom threshold component for threshold alert section
• Add validation for create infrastructure monitor page
• Add feature slice for infrastructure monitor that include create, get, update and delete monitors
• Add create infrastructure monitor page
• Fixed a css issue in CheckBox component that makes the lable and checkbox vertically off when in a row with other component
• Generate payload to create infrastructure monitor and able to save to BE with success
• Connect Infrastructure create page to global routes and hook up to "create monitor" button on infrastructure home page
• system load, cpu temperature and swap usage, log retention period and max tries are to be done , this will need backend support also, will be in another PR
  Screencast from 2024-11-19 09-58-57.webm

[gorkem-bwl]

Can you fix alignments here?

[ajhollid]

I don't see any major issues here, looks pretty good!

Minor issues:

1. I would make the util string funciton more robust
2. We need JSdoc for all functions/components.

Great work!

[jennifer-gan]

Can you fix alignments here?

I will see what I can do with the notification checkbox label and child element alignment; regarding the threshold part, it will be aligned when it is full size, when you shrink the screen, due to its responsive, you will see they will not be left align
after a certain size, but they will be right aligned always.

[ajhollid]

Nice work here, the UI looks really nice 👍

There's a few issues I noticed while running the code, I've noted them in the comments.

Aside from that there are some organizational changes I think we can make. Anything not reusable should be moved to the page specific directory, only reusable code/components should be refactored out to util files or the Component directory.

If component specific code is added to util files it just makes it harder to track down where logic is being executed.

It seems like something from @marcelluscaio's recent them refactor has affected the showing of errors, so we'll have to track that one down. This isn't specific to your so we can double check the error handling once we've got that one sorted out!

[jennifer-gan]

This is to address the alignment issue @gorkem-bwl brought up, only when label is not a string, it will need to be a bottom padding of 65px, also not sure if this should be move to any theme , or even make it into its own css file? as it is not worth to have a separate class just for this one css property ?

[ajhollid]

If css vars are absoultely required, perhaps it would make more sense to declare them in their component specific css files?

I think the ultimate goal here is to not use this css file at all

[jennifer-gan]

I can move it to its own css file, but I think the point to put it in the global index.css is for future re-usability, in case anywhere else will use the same value?
I am not totally sure about using rem for all pixel sizes, will that work well when 250px is equal to 18.67 rem e.g ?

[ajhollid]

The values for usage_cpu, usage_memory, and usage_disk should all be numbers, this will cause an error if the form is submitted with default values as validation type is number

[jennifer-gan]

that has to be a string, since the component has define it as string (value)

Field.propTypes = {
	type: PropTypes.oneOf(["text", "password", "url", "email", "description", "number"]),
	id: PropTypes.string.isRequired,
	name: PropTypes.string,
	label: PropTypes.string,
	https: PropTypes.bool,
	isRequired: PropTypes.bool,
	isOptional: PropTypes.bool,
	optionalLabel: PropTypes.string,
	autoComplete: PropTypes.string,
	placeholder: PropTypes.string,
	value: PropTypes.string.isRequired,
	onChange: PropTypes.func,
	onBlur: PropTypes.func,
	onInput: PropTypes.func,
	error: PropTypes.string,
	disabled: PropTypes.bool,
	hidden: PropTypes.bool,
	className: PropTypes.string
};

BE seems work fine with or without those values as part of payload

[ajhollid]

You can convert the value to a string before passing it to whatever component requires it to be a string 🤔

This state seems misleading as the value really should be a number.

Yes the backend will accept a request with or without these values as they are optional.

[marcelluscaio]

I think we are not supposed to use floats with MUI's spacing
https://mui.com/material-ui/customization/spacing/

[jennifer-gan]

like mentioned in the chat, the left alignment issue of the notification email text and its previous child elements were already resolved, i'm not sure what we are trying to solve here

[marcelluscaio]

This is something else. The point here is we are using spacing to have a rule for the whole project. If we pass floats here, we are bypassing that rule (the idea is that all spacing will be in a certain scale, in our case that is 2. When you pass .5, we are increasing space by 1)

[jennifer-gan]

not sure why it was 2.5 historically, what we should be doing with this now or there will be another PR to address all floating spacing?

[marcelluscaio]

I think you could just change it to 2 or 3. Or leave it that way, this is not the core of this pr

[marcelluscaio]

Why do we have error message hidden or not based on having a className?

[jennifer-gan]

because they all refer to the same single case: threshold for infrastructure

[marcelluscaio]

Sorry, that is not clear for me, can you explain?

[jennifer-gan]

If you use for threshold infrastructure scenario, then you need that class name and hide the error for each field

[marcelluscaio]

Classname is not specified in jsdocs. The way it is, as I understand this, if we instantiate this compoennt passing a className to it, to extend styling maybe(?) we will hide the errors.

[marcelluscaio]

What if we get a string in all caps?

[jennifer-gan]

caller's call

[marcelluscaio]

You could solve that by lowercasing the string before making the capitalize first letter operation

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)

Client/src/Pages/InfrastructureMonitors/CreateMonitor/index.jsx (2)

23-36: Consider using more explicit initial interval value

The initial interval value of 0.25 might be confusing without context. Consider using a named constant or adding a comment to explain that this represents 15 seconds when converted to minutes.

 const [infrastructureMonitor, setInfrastructureMonitor] = useState({
   url: "",
   name: "",
   notifications: [],
-  interval: 0.25,
+  interval: 0.25, // 15 seconds (0.25 minutes)
   cpu: false,
   usage_cpu: "",
   memory: false,
   usage_memory: "",
   disk: false,
   usage_disk: "",
   secret: "",
 });

---

384-392: Consider implementing maximum retries feature

The commented-out maximum retries field suggests an incomplete feature. Would you like me to help implement this feature or create a GitHub issue to track it?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 49f95f1 and 657a953.

📒 Files selected for processing (1)

• Client/src/Pages/InfrastructureMonitors/CreateMonitor/index.jsx (1 hunks)

🧰 Additional context used

🪛 Biome

Client/src/Pages/InfrastructureMonitors/CreateMonitor/index.jsx

[error] 161-161: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

🔇 Additional comments (3)

Client/src/Pages/InfrastructureMonitors/CreateMonitor/index.jsx (3)

1-22: LGTM! Imports are well-organized.

The imports are logically grouped and all dependencies appear to be necessary for the component's functionality.

---

343-345: Review temperature unit implementation

The TODO comment indicates that temperature monitoring is planned for a future PR, but the code already includes temperature unit logic. Consider removing the temperature-specific code until the feature is implemented.

---

130-150: 🛠️ Refactor suggestion

Refactor payload generation for better performance

The use of the delete operator can impact performance. Let's restructure the payload generation to avoid mutations.

 const generatePayload = (form) => {
   let thresholds = {};
+  const formData = { ...form };
   Object.keys(form)
     .filter((k) => k.startsWith(THRESHOLD_FIELD_PREFIX))
     .map((k) => {
       if (form[k]) thresholds[k] = form[k];
-      delete form[k];
-      delete form[k.substring(THRESHOLD_FIELD_PREFIX.length)];
     });

-  form = {
+  return {
-    ...form,
+    ...formData,
     description: form.name,
     teamId: user.teamId,
     userId: user._id,
     type: "hardware",
     notifications: infrastructureMonitor.notifications,
     thresholds,
-  };
-  return form;
+  };
 };

Likely invalid or redundant comment.

[coderabbitai]

🛠️ Refactor suggestion

Ensure consistent time unit conversion

Previous review comments indicate that other parts of the app use minutes as the base unit. Let's ensure consistency by converting seconds to minutes before applying MS_PER_MINUTE.

 const handleCreateInfrastructureMonitor = async (event) => {
   event.preventDefault();
   let form = {
     ...infrastructureMonitor,
     name:
       infrastructureMonitor.name === ""
         ? infrastructureMonitor.url
         : infrastructureMonitor.name,
-    interval: infrastructureMonitor.interval * MS_PER_MINUTE,
+    // Convert interval to minutes before applying MS_PER_MINUTE
+    interval: (infrastructureMonitor.interval / 60) * MS_PER_MINUTE,
   };

📝 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.

Suggested change

	const handleCreateInfrastructureMonitor = async (event) => {
	event.preventDefault();
	let form = {
	...infrastructureMonitor,
	name:
	infrastructureMonitor.name === ""
	? infrastructureMonitor.url
	: infrastructureMonitor.name,
	interval: infrastructureMonitor.interval * MS_PER_MINUTE,
	};
	delete form.notifications;
	if (hasValidationErrors(form, infrastructureMonitorValidation, setErrors)) {
	return;
	} else {
	const checkEndpointAction = await dispatch(
	checkInfrastructureEndpointResolution({ authToken, monitorURL: form.url })
	);
	if (checkEndpointAction.meta.requestStatus === "rejected") {
	createToast({
	body: "The endpoint you entered doesn't resolve. Check the URL again.",
	});
	setErrors({ url: "The entered URL is not reachable." });
	return;
	}
	const action = await dispatch(
	createInfrastructureMonitor({ authToken, monitor: generatePayload(form) })
	);
	if (action.meta.requestStatus === "fulfilled") {
	createToast({ body: "Infrastructure monitor created successfully!" });
	navigate("/infrastructure-monitors/create");
	} else {
	createToast({ body: "Failed to create monitor." });
	}
	}
	};
	const handleCreateInfrastructureMonitor = async (event) => {
	event.preventDefault();
	let form = {
	...infrastructureMonitor,
	name:
	infrastructureMonitor.name === ""
	? infrastructureMonitor.url
	: infrastructureMonitor.name,
	// Convert interval to minutes before applying MS_PER_MINUTE
	interval: (infrastructureMonitor.interval / 60) * MS_PER_MINUTE,
	};
	delete form.notifications;
	if (hasValidationErrors(form, infrastructureMonitorValidation, setErrors)) {
	return;
	} else {
	const checkEndpointAction = await dispatch(
	checkInfrastructureEndpointResolution({ authToken, monitorURL: form.url })
	);
	if (checkEndpointAction.meta.requestStatus === "rejected") {
	createToast({
	body: "The endpoint you entered doesn't resolve. Check the URL again.",
	});
	setErrors({ url: "The entered URL is not reachable." });
	return;
	}
	const action = await dispatch(
	createInfrastructureMonitor({ authToken, monitor: generatePayload(form) })
	);
	if (action.meta.requestStatus === "fulfilled") {
	createToast({ body: "Infrastructure monitor created successfully!" });
	navigate("/infrastructure-monitors/create");
	} else {
	createToast({ body: "Failed to create monitor." });
	}
	}
	};

🧰 Tools

🪛 Biome

[error] 161-161: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[marcelluscaio]

To solve the padding rpoblem, delete the padding bottom and add alignSelf: "flex-start" to the sx of the MuiCheckbox (before &:hover, on line 59 or 60

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (7)

Client/src/Pages/Infrastructure/index.jsx (2)

20-20: Mom's spaghetti moment: Let's catch those navigation errors!

The navigation could fail due to various reasons (like route not being ready). Consider adding error handling:

- navigate("/infrastructure/create");
+ try {
+   navigate("/infrastructure/create");
+ } catch (error) {
+   console.error("Failed to navigate to create infrastructure page:", error);
+   // Handle the error appropriately, maybe show a user-friendly message
+ }

---

Line range hint 5-9: Weak knees alert: JSDoc needs some love! 📝

The JSDoc comments could use some cleanup:

 /**
  * This is the Infrastructure monitoring page. This is a work in progress
  *
- * @param  - Define params.
+ * @returns {JSX.Element} The Infrastructure monitoring page component that allows users
+ *                        to create and view infrastructure monitors.
  */

Client/src/Pages/Infrastructure/CreateMonitor/CustomThreshold/index.jsx (2)

8-24: Yo, let's clean up this documentation, it's making my knees weak!

There's a typo in the word "correspondant" (should be "corresponding"). Also, consider reorganizing the param descriptions to group related fields together (checkbox-related, field-related, etc.).

- * group of CheckBox with a label and its correspondant threshold input field.
+ * group of CheckBox with a label and its corresponding threshold input field.

---

82-92: Arms are heavy - these PropTypes need more specificity!

The infrastructureMonitor and errors object PropTypes could be more specific about their expected shape.

-  infrastructureMonitor: PropTypes.object.isRequired,
-  errors: PropTypes.object.isRequired,
+  infrastructureMonitor: PropTypes.shape({
+    [PropTypes.string]: PropTypes.oneOfType([
+      PropTypes.bool,
+      PropTypes.number
+    ])
+  }).isRequired,
+  errors: PropTypes.shape({
+    [PropTypes.string]: PropTypes.string
+  }).isRequired,

Client/src/Components/Sidebar/index.jsx (1)

Line range hint 1-450: Mom's spaghetti moment: This component needs some cleanup! 🍝

The component has grown quite large (450+ lines) and handles multiple responsibilities. Consider breaking it down into smaller, more manageable components:

• Menu item component
• User profile component
• Navigation logic hooks

Example structure:

// SidebarMenuItem.jsx
const SidebarMenuItem = ({ item, collapsed, ...props }) => {
  // Menu item rendering logic
}

// UserProfile.jsx
const UserProfile = ({ user, collapsed, ...props }) => {
  // User profile section logic
}

// useNavigation.jsx
const useNavigation = () => {
  // Navigation and routing logic
}

Client/src/Pages/Infrastructure/CreateMonitor/index.jsx (2)

116-116: Implement phone number notification handling

There's a TODO comment indicating that phone number notifications need to be added. Including this feature will provide users with more notification options.

Would you like assistance in implementing the phone number notification functionality or opening a GitHub issue to track this task?

---

61-61: Use strict equality checks for better type safety

It's a good practice to use === and !== in JavaScript to prevent unexpected type coercion.

Consider updating the condition:

                 [id]: prev[id] == undefined && value == "on" ? true : !prev[id],
+                [id]: prev[id] === undefined && value === "on" ? true : !prev[id],

This ensures that comparisons are accurate and types are consistently handled.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 657a953 and 5774d52.

📒 Files selected for processing (5)

• Client/src/App.jsx (2 hunks)
• Client/src/Components/Sidebar/index.jsx (1 hunks)
• Client/src/Pages/Infrastructure/CreateMonitor/CustomThreshold/index.jsx (1 hunks)
• Client/src/Pages/Infrastructure/CreateMonitor/index.jsx (1 hunks)
• Client/src/Pages/Infrastructure/index.jsx (2 hunks)

🧰 Additional context used

🪛 Biome

Client/src/Pages/Infrastructure/CreateMonitor/index.jsx

[error] 161-161: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

🔇 Additional comments (7)

Client/src/Pages/Infrastructure/index.jsx (1)

3-3: Yo, this hook implementation is straight fire! 🔥

The useNavigate hook is properly imported and initialized at the component level, following React best practices.

Also applies to: 13-13

Client/src/Pages/Infrastructure/CreateMonitor/CustomThreshold/index.jsx (1)

26-36: Props looking clean, like mom's spaghetti! 🍝

The props destructuring and naming conventions are well-organized and follow React best practices.

Client/src/App.jsx (2)

13-13: Yo, this import is straight fire! 🔥

The import follows the established pattern and maintains proper module organization.

---

131-134: Mom's spaghetti says this route looks ready! 🍝

The route is properly protected and follows the existing routing patterns. Just one thing to verify fam!

Let's make sure the component exists and is exported correctly:

✅ Verification successful

Yo, this spaghetti's perfectly cooked! 🍝

The CreateInfrastructureMonitor component is properly exported as a default export from Client/src/Pages/Infrastructure/CreateMonitor/index.jsx, just the way mama makes it! The routing setup is clean and ready to serve.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the CreateInfrastructureMonitor component exists and is exported

# Check if the component file exists and exports the component
ast-grep --pattern 'export $_ CreateInfrastructureMonitor'

# Backup check using ripgrep
rg -l "CreateInfrastructureMonitor" "Client/src/Pages/Infrastructure/CreateMonitor"

Length of output: 198

---

Script:

#!/bin/bash
# Let's check the actual export in the component file
rg -A 2 "export" "Client/src/Pages/Infrastructure/CreateMonitor/index.jsx"

# Also check if it's a default or named export
ast-grep --pattern 'export default CreateInfrastructureMonitor'
ast-grep --pattern 'export const CreateInfrastructureMonitor'

Length of output: 350

Client/src/Components/Sidebar/index.jsx (3)

93-93: Yo dawg, the infrastructure path mapping looks solid! 🍝

The addition of infrastructure: "Dashboard" aligns perfectly with the menu structure where infrastructure is nested under Dashboard. This maintains consistency with the existing navigation pattern.

---

Line range hint 38-44: Heads up! Let's verify the infrastructure icon choice, fam! 🤔

Using the Integrations icon for the infrastructure section might confuse users since it's typically associated with third-party integrations. Consider creating a dedicated infrastructure icon for better visual distinction.

---

Line range hint 147-156: Yo, we need to check if the infrastructure route needs role-based access! 🔒

The code checks for role-based access for the Team menu item, but there's no similar check for the new infrastructure section. Consider adding role-based access control for infrastructure monitoring.

Consider implementing role-based access control for infrastructure monitoring features:

 { name: "Infrastructure", path: "infrastructure", icon: <Integrations /> },
+// Add role check similar to Team menu item
+if (!user.role?.includes("admin") && !user.role?.includes("superadmin")) {
+  return null;
+}

[coderabbitai]

⚠️ Potential issue

Palms are sweaty - we need input validation!

The number input field lacks min/max constraints, which could lead to invalid threshold values.

 <Field
     type="number"
     className="field-infrastructure-alert"
     id={fieldId}
+    inputProps={{
+      min: 0,
+      max: 100,
+      step: 1
+    }}
     value={infrastructureMonitor[fieldId]}
     onBlur={onFieldBlur}
     onChange={onFieldChange}
     error={errors[fieldId]}
     disabled={!infrastructureMonitor[checkboxId]}
 />

📝 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.

Suggested change

	<Field
	type="number"
	className="field-infrastructure-alert"
	id={fieldId}
	value={infrastructureMonitor[fieldId]}
	onBlur={onFieldBlur}
	onChange={onFieldChange}
	error={errors[fieldId]}
	disabled={!infrastructureMonitor[checkboxId]}
	></Field>
	<Field
	type="number"
	className="field-infrastructure-alert"
	id={fieldId}
	inputProps={{
	min: 0,
	max: 100,
	step: 1
	}}
	value={infrastructureMonitor[fieldId]}
	onBlur={onFieldBlur}
	onChange={onFieldChange}
	error={errors[fieldId]}
	disabled={!infrastructureMonitor[checkboxId]}
	/>

[coderabbitai]

🛠️ Refactor suggestion

Vomit on his sweater already - this width needs to be responsive!

The hardcoded width: "50%" might cause layout issues on different screen sizes. Consider using Material-UI's responsive breakpoints or CSS Grid.

 <Stack
     direction={"row"}
     sx={{
-        width: "50%",
+        width: {
+          xs: "100%",
+          sm: "75%",
+          md: "50%"
+        },
         justifyContent: "space-between",
         flexWrap: "wrap"         
     }}
 >

📝 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.

Suggested change

	<Stack
	direction={"row"}
	sx={{
	width: "50%",
	justifyContent: "space-between",
	flexWrap: "wrap"
	}}
	>
	<Stack
	direction={"row"}
	sx={{
	width: {
	xs: "100%",
	sm: "75%",
	md: "50%"
	},
	justifyContent: "space-between",
	flexWrap: "wrap"
	}}
	>

[coderabbitai]

🛠️ Refactor suggestion

Avoid using the delete operator to enhance performance and maintain immutability

Using the delete operator can negatively impact performance and may lead to unintended side effects by mutating the original object. Instead, consider creating a new object without the unwanted properties.

Refactor the code in generatePayload to avoid deleting properties from form:

             Object.keys(form)
                 .filter((k) => k.startsWith(THRESHOLD_FIELD_PREFIX))
-                .map((k) => {
+                .forEach((k) => {
                     if (form[k]) thresholds[k] = form[k];
-                    delete form[k];
-                    delete form[k.substring(THRESHOLD_FIELD_PREFIX.length)];
+                });
+
+            const cleanedForm = Object.keys(form).reduce((acc, key) => {
+                if (
+                    !key.startsWith(THRESHOLD_FIELD_PREFIX) &&
+                    !HARDWARE_MONITOR_TYPES.includes(key)
+                ) {
+                    acc[key] = form[key];
+                }
+                return acc;
+            }, {});

Similarly, in line 161:

-        delete form.notifications;
+        const { notifications, ...cleanedForm } = form;

By avoiding direct mutations, you improve performance and ensure the original form object remains immutable.

Also applies to: 161-161

[coderabbitai]

⚠️ Potential issue

Ensure the default interval value matches the available options

The default value for infrastructureMonitor.interval is set to 15, but the frequencies array only includes values up to 10 minutes. This mismatch might cause the default selection to display an invalid option.

Update the default value to align with the frequencies array. If you intend the default to be "15 seconds" (0.25):

             value={infrastructureMonitor.interval || 15}
+            value={infrastructureMonitor.interval || 0.25}

This ensures the default interval corresponds to a valid option in the dropdown.

Also applies to: 379-379

[ajhollid]

I think this is in a decent enough state to merge in.

There are a few issues here and there but I think we would be better off merging this in now and addressing those fixes in a separate PR.