Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Create monitor display name v2, resolves #992 #1234

Merged
merged 8 commits into from
Dec 2, 2024
Merged

feat: Create monitor display name v2, resolves #992 #1234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4e2303b
Add domain parsing function
ajhollid Nov 27, 2024
ea4ddfc
set name field onBlur when URL is changed
ajhollid Nov 27, 2024
9e45297
Merge remote-tracking branch 'upstream/develop' into develop
peterpardo Nov 27, 2024
f50748e
Merge remote-tracking branch 'upstream/develop' into develop
peterpardo Nov 29, 2024
2b24f19
Updated domain parsing logic
peterpardo Nov 30, 2024
5678771
Always update display name on when URL changes
peterpardo Nov 30, 2024
fb13fb1
Merge remote-tracking branch 'upstream/develop' into develop
peterpardo Nov 30, 2024
0adc8fd
Merge branch 'develop' into feat/create-monitor-display-name-v2
peterpardo Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Client/src/Pages/Infrastructure/CreateMonitor/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Box, Stack, Typography } from "@mui/material";
import LoadingButton from "@mui/lab/LoadingButton";
import { useSelector, useDispatch } from "react-redux";
import { infrastructureMonitorValidation } from "../../../Validation/validation";
import { parseDomainName } from "../../../Utils/monitorUtils";
import {
createInfrastructureMonitor,
checkInfrastructureEndpointResolution,
Expand Down Expand Up @@ -79,6 +80,15 @@ const CreateInfrastructureMonitor = () => {
const handleBlur = (event, appendID) => {
event.preventDefault();
const { value, id } = event.target;

let name = idMap[id] ?? id;
if (name === "url" && infrastructureMonitor.name === "") {
setInfrastructureMonitor((prev) => ({
...prev,
name: parseDomainName(value),
}));
}

Comment on lines +83 to +91
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and consider extracting common URL blur logic.

Yo! The URL blur logic is duplicated across components and lacks error handling.

Consider these improvements:

  1. Add error handling:
 if (name === "url" && infrastructureMonitor.name === "") {
+  try {
     setInfrastructureMonitor((prev) => ({
       ...prev,
       name: parseDomainName(value),
     }));
+  } catch (error) {
+    createToast({ body: "Failed to generate display name from URL" });
+  }
 }
  1. Extract common logic to a custom hook:
// hooks/useUrlDisplayName.js
export const useUrlDisplayName = (setValue) => {
  const handleUrlBlur = (event) => {
    const { value } = event.target;
    try {
      setValue((prev) => ({
        ...prev,
        name: parseDomainName(value),
      }));
    } catch (error) {
      createToast({ body: "Failed to generate display name from URL" });
    }
  };
  return handleUrlBlur;
};

if (id?.startsWith("notify-email-")) return;
const { error } = infrastructureMonitorValidation.validate(
{ [id ?? appendID]: value },
Expand Down
12 changes: 11 additions & 1 deletion Client/src/Pages/Monitors/CreateMonitor/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Checkbox from "../../../Components/Inputs/Checkbox";
import Breadcrumbs from "../../../Components/Breadcrumbs";
import { getUptimeMonitorById } from "../../../Features/UptimeMonitors/uptimeMonitorsSlice";
import "./index.css";
import { parseDomainName } from "../../../Utils/monitorUtils";

const CreateMonitor = () => {
const MS_PER_MINUTE = 60000;
Expand Down Expand Up @@ -91,7 +92,7 @@ const CreateMonitor = () => {
}
};
fetchMonitor();
}, [monitorId, authToken, monitors]);
}, [monitorId, authToken, monitors, dispatch, navigate]);

const handleChange = (event, name) => {
const { value, id } = event.target;
Expand Down Expand Up @@ -141,6 +142,14 @@ const CreateMonitor = () => {
}
};

const onUrlBlur = (event) => {
const { value } = event.target;
setMonitor((prev) => ({
...prev,
name: parseDomainName(value),
}));
};
Comment on lines +145 to +151
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Yo dawg, we need some error handling in this function!

The onUrlBlur function's looking clean, but it's raw-dogging the state update without any error handling. What if parseDomainName throws?

Consider wrapping it in a try-catch like this:

 const onUrlBlur = (event) => {
   const { value } = event.target;
+  try {
     setMonitor((prev) => ({
       ...prev,
       name: parseDomainName(value),
     }));
+  } catch (error) {
+    createToast({
+      body: "Failed to generate display name from URL",
+    });
+    logger.error("Error parsing domain name:", error);
+  }
 };

Committable suggestion skipped: line range outside the PR's diff.


const handleCreateMonitor = async (event) => {
event.preventDefault();
//obj to submit
Expand Down Expand Up @@ -259,6 +268,7 @@ const CreateMonitor = () => {
placeholder={monitorTypeMaps[monitor.type].placeholder || ""}
value={monitor.url}
onChange={handleChange}
onBlur={onUrlBlur}
error={errors["url"] ? true : false}
helperText={errors["url"]}
/>
Expand Down
12 changes: 12 additions & 0 deletions Client/src/Pages/PageSpeed/CreatePageSpeed/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { HttpAdornment } from "../../../Components/Inputs/TextInput/Adornments";
import Select from "../../../Components/Inputs/Select";
import Checkbox from "../../../Components/Inputs/Checkbox";
import Breadcrumbs from "../../../Components/Breadcrumbs";
import { parseDomainName } from "../../../Utils/monitorUtils";
import "./index.css";

const CreatePageSpeed = () => {
Expand Down Expand Up @@ -95,6 +96,16 @@ const CreatePageSpeed = () => {
}
};

const onUrlBlur = (event) => {
const { value } = event.target;
if (monitor.name === "") {
setMonitor((prev) => ({
...prev,
name: parseDomainName(value),
}));
}
};
Comment on lines +99 to +107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to the URL blur handler.

The handler needs to catch potential errors from parseDomainName to prevent crashes.

Here's the fix:

 const onUrlBlur = (event) => {
   const { value } = event.target;
   if (monitor.name === "") {
+    try {
       setMonitor((prev) => ({
         ...prev,
         name: parseDomainName(value),
       }));
+    } catch (error) {
+      createToast({ body: "Failed to generate display name from URL" });
+    }
   }
 };

Committable suggestion skipped: line range outside the PR's diff.


const handleCreateMonitor = async (event) => {
event.preventDefault();
//obj to submit
Expand Down Expand Up @@ -214,6 +225,7 @@ const CreatePageSpeed = () => {
placeholder="google.com"
value={monitor.url}
onChange={handleChange}
onBlur={onUrlBlur}
error={errors["url"] ? true : false}
helperText={errors["url"]}
/>
Expand Down
21 changes: 21 additions & 0 deletions Client/src/Utils/monitorUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { capitalizeFirstLetter } from "./stringUtils";

/**
* Helper function to get duration since last check or the last date checked
* @param {Array} checks Array of check objects.
Expand All @@ -15,3 +17,22 @@ export const getLastChecked = (checks, duration = true) => {
}
return new Date() - new Date(checks[0].createdAt);
};

export const parseDomainName = (url) => {
url = url.replace(/^https?:\/\//, "");
// Remove leading/trailing dots
url = url.replace(/^\.+|\.+$/g, "");
// Split by dots
const parts = url.split(".");
// Remove common prefixes and empty parts and exclude the last element of the array (the last element should be the TLD)
const cleanParts = parts.filter((part) => part !== "www" && part !== "").slice(0, -1);
// If there's more than one part, append the two words and capitalize the first letters (e.g. ["api", "test"] -> "Api Test")
const domainPart =
cleanParts.length > 1
? cleanParts.map((part) => capitalizeFirstLetter(part)).join(" ")
: capitalizeFirstLetter(cleanParts[0]);

if (domainPart) return domainPart;

return url;
};
Comment on lines +21 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and documentation for the parseDomainName function.

Yo! The function's logic is solid, but it needs some defensive programming and documentation to make it bulletproof.

Here's how we can improve it:

+/**
+ * Parses a URL and generates a display name by extracting and formatting the domain parts.
+ * @param {string} url - The URL to parse (e.g., "www.api.test.com")
+ * @returns {string} - Formatted display name (e.g., "Api Test")
+ * @throws {Error} - If url is not a string or is empty
+ */
 export const parseDomainName = (url) => {
+  if (typeof url !== 'string' || !url.trim()) {
+    throw new Error('URL must be a non-empty string');
+  }
+
   url = url.replace(/^https?:\/\//, "");
   // Remove leading/trailing dots
   url = url.replace(/^\.+|\.+$/g, "");
   // Split by dots
   const parts = url.split(".");
   // Remove common prefixes and empty parts and exclude the last element of the array
   const cleanParts = parts.filter((part) => part !== "www" && part !== "").slice(0, -1);
   // If there's more than one part, append the two words and capitalize
   const domainPart =
     cleanParts.length > 1
       ? cleanParts.map((part) => capitalizeFirstLetter(part)).join(" ")
       : capitalizeFirstLetter(cleanParts[0]);

-  if (domainPart) return domainPart;
-
-  return url;
+  return domainPart || url;
 };
📝 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
export const parseDomainName = (url) => {
url = url.replace(/^https?:\/\//, "");
// Remove leading/trailing dots
url = url.replace(/^\.+|\.+$/g, "");
// Split by dots
const parts = url.split(".");
// Remove common prefixes and empty parts and exclude the last element of the array (the last element should be the TLD)
const cleanParts = parts.filter((part) => part !== "www" && part !== "").slice(0, -1);
// If there's more than one part, append the two words and capitalize the first letters (e.g. ["api", "test"] -> "Api Test")
const domainPart =
cleanParts.length > 1
? cleanParts.map((part) => capitalizeFirstLetter(part)).join(" ")
: capitalizeFirstLetter(cleanParts[0]);
if (domainPart) return domainPart;
return url;
};
/**
* Parses a URL and generates a display name by extracting and formatting the domain parts.
* @param {string} url - The URL to parse (e.g., "www.api.test.com")
* @returns {string} - Formatted display name (e.g., "Api Test")
* @throws {Error} - If url is not a string or is empty
*/
export const parseDomainName = (url) => {
if (typeof url !== 'string' || !url.trim()) {
throw new Error('URL must be a non-empty string');
}
url = url.replace(/^https?:\/\//, "");
// Remove leading/trailing dots
url = url.replace(/^\.+|\.+$/g, "");
// Split by dots
const parts = url.split(".");
// Remove common prefixes and empty parts and exclude the last element of the array
const cleanParts = parts.filter((part) => part !== "www" && part !== "").slice(0, -1);
// If there's more than one part, append the two words and capitalize
const domainPart =
cleanParts.length > 1
? cleanParts.map((part) => capitalizeFirstLetter(part)).join(" ")
: capitalizeFirstLetter(cleanParts[0]);
return domainPart || url;
};