Client/src/Pages/Monitors/CreateMonitor/index.jsx (1)

177-178: Provide more specific error feedback

The error message "The entered URL is not reachable." might leave users feeling queasy without clear guidance. Enhance the message to help users troubleshoot effectively.

Consider specifying why the URL might not be reachable and suggest checking their internet connection or URL correctness.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/routes/monitorRoute.js (1)

21-25: Yo, this endpoint's lookin' fresh, dawg! But let's make it bulletproof, ya feel me?

The new endpoint for checkin' if the URL resolves is solid gold, homie. It's locked down tight with that isAllowed middleware, just like the other VIP routes. But yo, we gotta think about what happens if somethin' goes sideways, you know what I'm sayin'?

How 'bout we add some error handling to this bad boy? Check it:

 router.get(
   "/check-endpoint/url",
   isAllowed(["admin", "superadmin"]), 
-  monitorController.checkEndpointResolution
+  async (req, res, next) => {
+    try {
+      await monitorController.checkEndpointResolution(req, res);
+    } catch (error) {
+      next(error);
+    }
+  }
 )

This way, if the controller throws up (pun intended, mom's spaghetti), we catch that error and pass it to the next middleware. It's like havin' a safety net for our code, you feel me?

Client/src/Features/UptimeMonitors/uptimeMonitorsSlice.js (2)

34-56: Yo, this thunk's got the flow, but let's make it glow!

The new checkEndpointResolution thunk is lookin' fresh, followin' the same beat as the other thunks in this track. It's handlin' the authToken and monitorURL like a pro, just what we needed for this endpoint check feature.

But yo, check this out - we could make the error handlin' even smoother. Instead of repeatin' ourselves like a broken record, how 'bout we create a helper function for that error logic? It'd make our code as clean as mom's spaghetti (before it ended up on the sweater, ya feel me?).

Wanna see how we could clean this up? Just say the word, and I'll drop some code that'll make this error handling slicker than Eminem's rhymes.

🧰 Tools

🪛 Biome

[error] 46-46: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

298-314: These reducers are fire, but let's turn up the heat!

Yo, these new reducers for checkEndpointResolution are droppin' beats in perfect sync with the rest of the track. They're handlin' the state like a pro DJ, keepin' that isLoading, success, and msg in perfect harmony.

But check it, we got a chance to make our error messages as consistent as a metronome. Peep this:

state.msg = action.payload?.msg ?? "Failed to check endpoint resolution";

This way, we're catchin' all the vibes, whether the payload's got a message or not. It's like havin' a backup track ready to drop if the main one cuts out, you feel me?

If you want me to lay down this beat across all the reducers, just give the word. We'll have this code flowin' smoother than Eminem's rhymes in no time!

Server/controllers/monitorController.js (1)

277-301: Yo, this function's droppin' beats like it's hot!

This checkEndpointResolution function's got the flow, ya feel me? It's sanitizin' URLs like a clean freak and usin' both Google and Cloudflare DNS servers like it's playin' both sides of the track. That's some next-level reliability right there!

But yo, we got a couple things to polish up:

1. That console.log on line 288 is like leavin' the mic on during a bathroom break. Let's cut that noise out in the final mix, ya dig?

2. The error handlin' could use some extra flavor. Right now it's just throwin' generic errors like they're confetti. Let's break it down and give some specific error messages dependin' on what went wrong. It'll help the devs debug faster than Eminem spits rhymes.

Overall though, this function's got mad potential. It's gonna make our endpoint checks tighter than mom's spaghetti!

Client/src/Utils/NetworkService.js (1)

92-116: Yo, this new method's got potential, but it's shakin' like mom's spaghetti!

The checkEndpointResolution method looks solid overall, but there's a chance it might choke if we ain't careful. Here's what's cookin':

1. We're missin' some error handling, yo. What if authToken or monitorURL are empty? We gotta catch that before it blows up in our face.
2. That URLSearchParams usage? That's the real MVP right there. Handles them query params like a boss.
3. The method's structure is tight, followin' the flow of the other methods in this class. That's consistency, and consistency is key, ya feel me?

Here's a suggestion to make it bulletproof:

 async checkEndpointResolution(config) {
   const { authToken, monitorURL } = config;
+  if (!authToken || !monitorURL) {
+    throw new Error("authToken and monitorURL are required, don't leave me hangin'!");
+  }
   const params = new URLSearchParams();

   if (monitorURL) params.append("monitorURL", monitorURL);

   return this.axiosInstance.get(`/monitors/check-endpoint/url?${params.toString()}`, {
     headers: {
       Authorization: `Bearer ${authToken}`,
       "Content-Type": "application/json",
     }
   })
 }

This way, we're checkin' if we got all the ingredients before we start cookin'. Don't want no half-baked requests, you know what I'm sayin'?

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/tests/controllers/monitorController.test.js (1)

478-528: Add a test case for invalid URLs

Before we lose ourselves, consider adding a test case with an invalid URL (e.g., 'http://invalid-url'). This ensures the checkEndpointResolution function handles such scenarios gracefully without choking.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/routes/monitorRoute.js (1)

30-35: Yo, this new route is fire, but let's make it even hotter!

Aight, listen up! This new route for checkin' if the URL resolves is dope. It's like you're checkin' if the mic's on before you start spittin' bars. But yo, we could make it even better.

How 'bout we add some error handling? Like, what if the controller throws up (I mean, throws an error)? We don't want our users to see a 500 error, that's whack. Let's wrap it in a try-catch, ya feel me?

Here's how we could do it:

router.get(
  "/resolution/url",
  isAllowed(["admin", "superadmin"]),
  async (req, res, next) => {
    try {
      await monitorController.checkEndpointResolution(req, res, next);
    } catch (error) {
      console.error("Error checking endpoint resolution:", error);
      res.status(500).json({ error: "Failed to check endpoint resolution" });
    }
  }
);

This way, if somethin' goes wrong, we catch it like a pro and send back a clean response. It's like having a hype man to cover for you if you stumble on stage, you know what I'm sayin'?

Client/src/Features/UptimeMonitors/uptimeMonitorsSlice.js (3)

34-56: Yo, this function's lookin' fresh, but let's spice up that error message!

The implementation of checkEndpointResolution is solid, following the same structure as other async thunks. Props for fixin' that typo, homie! But check it, we could make the error message more specific to keep it consistent with the vibe of the function.

Consider changin' the generic error message to somethin' more on point, like:

-        msg: error.message ? error.message : "Unknown error",
+        msg: error.message ? error.message : "Failed to check endpoint resolution",

This way, if somethin' goes sideways, we know exactly what went down, ya feel me?

🧰 Tools

🪛 Biome

[error] 46-46: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

298-315: Aight, these new cases are tight, but let's keep 'em organized!

The new cases for checkEndpointResolution are on point, followin' the same flow as the other async thunks. You're handlin' the state like a pro, no cap!

To keep things clean and fresh, consider movin' these new cases right after the "Create Monitor" section. It's all about that logical flow, you know what I'm sayin'? Like this:

// Create Monitor
// ...

// Resolve Endpoint
// ...

// Get Monitor By Id
// ...

This way, we're keepin' related stuff together, makin' it easier for the next dev to navigate this code jungle. It's all about that readability, fam!

---

Line range hint 34-314: Yo, this code's lookin' fresh to death! Let's wrap it up!

Overall, you've done a solid job implementin' the checkEndpointResolution functionality. It's integrated smoothly into the existing code structure, and you're handlin' errors like a boss. Mad respect for keepin' things consistent!

If you're down to take this to the next level, I got some ideas:

1. We could add some unit tests to make sure this new function is bulletproof.
2. Maybe we could optimize the error handling across all these async thunks to reduce code duplication.

You want me to whip up some examples for either of these? Just say the word, and I'll cook up some code faster than mom's spaghetti! 🍝

🧰 Tools

🪛 Biome

[error] 46-46: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Server/openapi.json (1)

848-902: Yo, this endpoint's lookin' fresh, but it needs some sauce, ya feel me?

Aight, check it, the new endpoint's got potential, but it's missin' some flavor. Here's what we gotta do to make it pop:

1. The description's weak, knees weak, arms are heavy. Let's beef it up with more details 'bout what this endpoint's all about.
2. That example URL? It's basic like mom's spaghetti. Let's swap it for somethin' more real-world, ya dig?
3. When things go south (400 response), we gotta give more info. What kinda DNS resolution failures we talkin' 'bout?

Here's how we can spice it up:

 "/monitors/resolution/url": {
   "get": {
     "tags": ["monitors"],
-    "description": "Check DNS resolution for a given URL",
+    "description": "Validates the DNS resolution of a specified URL, ensuring the endpoint is reachable before adding a monitor",
     "parameters": [
       {
         "name": "monitorURL",
         "in": "query",
         "required": true,
         "schema": {
           "type": "string",
-          "example": "https://example.com"
+          "example": "https://api.bluewavelabs.ca"
         },
-        "description": "The URL to check DNS resolution for"
+        "description": "The URL to validate DNS resolution for, including protocol (http:// or https://)"
       }
     ],
     "responses": {
       "200": {
         "description": "URL resolved successfully",
         "content": {
           "application/json": {
             "schema": {
               "$ref": "#/components/schemas/SuccessResponse"
             }
           }
         }
       },
       "400": {
-        "description": "DNS resolution failed",
+        "description": "DNS resolution failed (e.g., non-existent domain, network issues)",
         "content": {
           "application/json": {
             "schema": {
               "$ref": "#/components/schemas/ErrorResponse"
             }
           }
         }
       },
       // ... (rest of the code remains unchanged)
     }
   }
 }

Server/controllers/monitorController.js (1)

266-275: Update JSDoc comments to reflect the function's behaviour

The JSDoc comments mention that the function adds the endpoint to the job queue and refers to validation errors with a 422 status code. However, the current implementation does not add anything to the job queue, and validation errors return a 400 status code. Consider updating the comments to accurately describe the function's purpose and error handling.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/routes/monitorRoute.js (2)

31-35: This new route's got me weak in the knees!

Yo, this new GET route for checkin' endpoint resolution is tighter than mom's spaghetti! It's locked down for the big dogs (admin and superadmin), which is smart like Eminem's lyrics.

But hey, let's make it even smoother. How about we use ES6 object property shorthand? It'll make our code flow like a rap battle:

router.get(
  "/resolution/url",
  isAllowed(["admin", "superadmin"]),
  checkEndpointResolution
);

This way, it's all neat and clean, no extra characters to trip over. What do you say, homie?

---

Line range hint 1-54: Alright, let's wrap this up like a tight verse!

Yo, we've been through this code like Eminem goes through rhymes, and here's the lowdown:

1. Those import statements? They're organized better than mom's spaghetti recipe now.
2. We've got a fresh new route for checkin' endpoint resolution - it's locked down tight for the admins.
3. But we've got a case of déjà vu with those delete routes. We gotta drop one like a weak beat to keep our flow smooth.

Overall, this code's got potential to be a chart-topper. Just need to clean up that duplicate delete route, and we'll be ready to drop this hot track... I mean, merge this PR.

Keep it real, keep it clean, and let's make this code flow like the dopest rhymes!

Server/controllers/monitorController.js (1)

22-25: Remove unused imports if not necessary.

Ensure that all imported modules are being utilized in your code. If handleError and handleValidationError from "./controllerUtils.js" or dns from "dns" are not used elsewhere, consider removing them to keep your code lean.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Client/src/Pages/Monitors/CreateMonitor/index.jsx (1)

Line range hint 129-166: Endpoint check's got mad flow, dawg!

Yo, this handleCreateMonitor function's got more layers than an onion! You're checkin' that endpoint like a boss, handlin' errors like a pro. It's beautiful, man, brings a tear to my eye!

But hey, one small thing to make it even tighter:

Consider extracting the endpoint check logic into a separate function. It'll make your code cleaner than your mom's kitchen after spaghetti night! Something like:

const checkEndpoint = async (url) => {
  const checkEndpointAction = await dispatch(
    checkEndpointResolution({ authToken, monitorURL: url })
  );
  return checkEndpointAction.meta.requestStatus !== "rejected";
};

Then you can use it in handleCreateMonitor like:

if (monitor.type === "http") {
  setIsCheckingEndpoint(true);
  const isEndpointValid = await checkEndpoint(form.url);
  setIsCheckingEndpoint(false);
  if (!isEndpointValid) {
    createToast({ body: "The endpoint you entered doesn't resolve. Check the URL again." });
    setErrors({ url: "The entered URL is not reachable." });
    return;
  }
}

This'll make your code flow smoother than your rhymes, homie!

Server/controllers/monitorController.js (1)

266-302: Yo, this checkEndpointResolution function is straight fire!

Mad props for dropping this sick function, homie! It's handling that URL resolution like a boss. But yo, we gotta make sure we don't choke on bad URLs, you feel me?

Let's beef up that URL parsing game:

  try {
    let { monitorURL } = req.query;
-   monitorURL = new URL(monitorURL);
+   try {
+     monitorURL = new URL(monitorURL);
+   } catch (urlError) {
+     return res.status(400).json({
+       success: false,
+       msg: `Invalid URL format: ${urlError.message}`,
+     });
+   }
    await new Promise((resolve, reject) => {
      dns.resolve(monitorURL.hostname, (error) => {

This way, we ain't gonna slip on any URL banana peels, you know what I'm sayin'?

Server/tests/controllers/monitorController.test.js (1)

462-462: Yo, dawg! Let's fix that typo faster than Eminem drops rhymes!

There's a typo in the test suite name. It's spelled "Controllor" but it should be "Controller". Let's clean that up like we're cleaning up after a rap battle.

-describe("Monitor Controllor - checkEndpointResolution", () => {
+describe("Monitor Controller - checkEndpointResolution", () => {

Server/openapi.json (1)

848-912: Yo, this endpoint's lookin' fresh, dawg! But we could spice it up a bit, ya feel me?

The new /monitors/resolution/url endpoint is solid, following the API's vibe and best practices. It's got that sweet bearer auth, proper response codes, and it's usin' query params like a boss. But yo, we could make it even doper by addin' some example values, know what I'm sayin'?

Here's a lil' somethin' to make it pop:

 "parameters": [
   {
     "name": "monitorURL",
     "in": "query",
     "required": true,
     "schema": {
       "type": "string",
-      "example": "https://example.com"
+      "example": "https://bluewavelabs.ca"
     },
     "description": "The URL to check DNS resolution for"
   }
 ],

This example's gonna make devs feel right at home, you dig?

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/tests/controllers/monitorController.test.js (3)

473-482: This test case is fire, but we can make it hotter than Mom's spaghetti!

The test for successful URL resolution is solid, but let's make it as precise as Eminem's lyrics. We can check the exact response structure to make sure we're not missing any beats.

Here's a suggestion to make it even tighter:

 expect(res.status.calledWith(200)).to.be.true;
-expect(res.json.calledWith({
-	success: true,
-	msg: 'URL resolved successfully',
-})).to.be.true;
+expect(res.json.calledWith(sinon.match({
+	success: true,
+	msg: 'URL resolved successfully'
+}))).to.be.true;
 expect(next.called).to.be.false;

This way, we're making sure the response is exactly what we expect, just like how every word in Eminem's verses is carefully chosen.

---

483-494: This error handling is almost as smooth as Eminem's flow, but we can make it smoother!

Your test for DNS resolution failure is solid, but let's make it as precise as Mom's spaghetti recipe. We can tighten up those assertions to make sure we're catching every detail.

Here's how we can make it even tighter:

 expect(next.calledOnce).to.be.true;
 const errorPassedToNext = next.getCall(0).args[0];
 expect(errorPassedToNext).to.be.an.instanceOf(Error);
-expect(errorPassedToNext.message).to.include('DNS resolution failed');
+expect(errorPassedToNext.message).to.equal('DNS resolution failed');
 expect(errorPassedToNext.code).to.equal('ENOTFOUND');
-expect(errorPassedToNext.status).to.equal(500);
+expect(errorPassedToNext.status).to.strictly.equal(500);

Now we're checking for the exact error message and using strict equality for the status code. It's all about them details, you feel me?

---

495-503: This validation test is weaker than Mom's spaghetti! Let's beef it up!

Your test for query validation is a good start, but we can make it as versatile as Eminem's vocabulary. Let's test multiple invalid URL scenarios and tighten up those assertions.

Check out this improved version:

-it('should reject with an error if query validation fails', async () => {
-	req.query.monitorURL = 'invalid-url';
-	await checkEndpointResolution(req, res, next);
-	expect(next.calledOnce).to.be.true;
-	const error = next.getCall(0).args[0];
-	expect(next.firstCall.args[0]).to.be.an("error");
-	expect(next.firstCall.args[0].status).to.equal(422);
-	expect(error.message).to.equal('"monitorURL" must be a valid uri');
-});
+it('should reject with an error if query validation fails', async () => {
+	const invalidUrls = ['invalid-url', 'http://', 'https://', 'ftp://example.com'];
+	for (const invalidUrl of invalidUrls) {
+		req.query.monitorURL = invalidUrl;
+		await checkEndpointResolution(req, res, next);
+		expect(next.calledOnce).to.be.true;
+		const error = next.getCall(0).args[0];
+		expect(error).to.be.an("error");
+		expect(error.status).to.strictly.equal(422);
+		expect(error.message).to.equal('"monitorURL" must be a valid uri');
+		next.reset();
+	}
+});

Now we're testing multiple invalid URLs, making sure our validation is as versatile as Eminem's rhymes!

Server/controllers/monitorController.js (1)

296-298: Maintain consistent success message formatting.

Flow's good, but let's keep the rhythm tight. Consider using successMessages for your response message to stay in sync with the rest of the code.

Apply this diff to update the message:

-        msg: `URL resolved successfully`,
+        msg: successMessages.URL_RESOLUTION_SUCCESS,

Don't forget to define URL_RESOLUTION_SUCCESS in your successMessages.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Server/openapi.json (1)

916-981: Yo, this new endpoint's got me weak in the knees, it's so fly!

This fresh /monitors/resolution/url endpoint is droppin' some serious heat, checkin' DNS resolution like it's nobody's business. It's got all the right moves:

• GET request? Check.
• monitorURL parameter? You know it.
• Responses for all scenarios? It's covered like mom's spaghetti on a sweater.

But yo, let's kick it up a notch. How about we add some example values for those response schemas? It'll make the devs' palms less sweaty when they're trying to implement this bad boy.

Client/src/Pages/PageSpeed/CreatePageSpeed/index.jsx (2)

346-346: Simplify the disabled condition for cleaner code

The expression Object.keys(errors).length !== 0 && true contains an unnecessary && true. Simplifying it keeps the code clean and prevents any unexpected slips.

Apply this diff to streamline the condition:

- disabled={Object.keys(errors).length !== 0 && true}
+ disabled={Object.keys(errors).length !== 0}

---

115-115: Improve the validation error message for better user experience

The toast message "Error validation data." might be unclear to users. A more descriptive message can help prevent any confusion, keeping everything running smoothly.

Apply this diff to enhance the error message:

- createToast({ body: "Error validation data." });
+ createToast({ body: "Please correct the highlighted errors before proceeding." });

Configuration used: .coderabbit.yaml
Review profile: CHILL