feat: allow updating monitor URL and type via Configure #2752
Conversation
Yasir-Rafique
requested review from
ajhollid,
mohicody,
Br0wnHammer,
Owaiseimdad,
karenvicent and
shanikauwu1
as code owners
WalkthroughClient enables editing monitor URL and protocol in Infrastructure edit UI, updates monitor hook to include url/port/gameId in update payloads for relevant types, and server Joi schemas now validate url (http/https) on create and edit and add gameId support; Infrastructure form builds normalized thresholds on submit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as Infrastructure Page (client)
participant H as useUpdateMonitor / submit handler
participant API as Server API
participant V as Joi Validator
U->>UI: Edit monitor (change URL/protocol / port / gameId)
UI->>H: onSubmit(form)
note over H: Build thresholds (usage_* ÷ 100)<br/>Compose payload: url (with scheme), port?, gameId?, type, description
H->>API: PUT /monitors/:id { url, port?, gameId?, thresholds, ... }
API->>V: Validate body (url URI http/https, optional url, gameId presence)
V-->>API: Valid / Invalid
alt Valid
API-->>H: 200 OK
H-->>UI: navigate/refresh
else Invalid
API-->>H: 400 Validation error
H-->>UI: show validation errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
P.S. A Canadian once told an American: “We apologize for being polite about winning—eh?” Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
1. Overview
1.1 Core Changes
- Primary purpose and scope: Enable editing monitor URLs and protocols (HTTP/HTTPS) without recreating monitors, preserving historical data
- Key components modified: Frontend UI components, backend validation, and API communication
- Cross-component impacts: Affects monitor configuration UI, API validation, and database updates
- Business value alignment: Addresses user pain point of losing monitoring history when making simple URL/protocol changes
1.2 Technical Architecture
- System design modifications: Extends existing monitor update functionality with URL/protocol editing capability
- Component interaction changes: Frontend now constructs full URLs before submission to backend
- Integration points impact: API now accepts optional URL field during monitor updates
- Dependency changes and implications: No new dependencies added, maintains existing architecture
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Double-protocol vulnerability in URL construction
- Analysis Confidence: High
- Impact: Could lead to malformed URLs causing monitoring failures or potential security vulnerabilities
- Resolution: Implement URL normalization utility to prevent double protocols
Issue: Insecure backend validation for URL field
- Analysis Confidence: High
- Impact: Accepts invalid/malicious URLs that could disrupt monitoring or cause security issues
- Resolution: Strengthen validation using joi.uri() with scheme restrictions
2.2 Should Fix (P1🟡)
Issue: Missing tests for URL/protocol update functionality
- Analysis Confidence: High
- Impact: Untested edge cases could lead to production issues with URL updates
- Suggested Solution: Add comprehensive tests for URL normalization, validation scenarios, and protocol transitions
Issue: Protocol handling logic duplication
- Analysis Confidence: Medium
- Impact: Maintenance challenges and potential inconsistencies
- Suggested Solution: Centralize protocol handling in utility functions
Issue: No specific error handling for invalid URL formats
- Analysis Confidence: Medium
- Impact: Poor user experience when invalid URLs are submitted
- Suggested Solution: Add client-side validation and user feedback for invalid URLs
2.3 Consider (P2🟢)
Area: Future protocol support
- Analysis Confidence: Medium
- Improvement Opportunity: Design protocol handling to accommodate future protocol additions
Area: Monitoring for update failures
- Analysis Confidence: Low
- Improvement Opportunity: Add monitoring points for failed monitor updates
Area: Component separation
- Analysis Confidence: Medium
- Improvement Opportunity: Improve separation between create and edit functionality in UI components
2.4 Summary of Action Items
- Immediately address P0 issues (double-protocol vulnerability and insecure validation)
- Add comprehensive test coverage for URL handling functionality
- Centralize protocol handling logic
- Consider future improvements for protocol support and monitoring
3. Technical Analysis
3.1 Code Logic Analysis
📁 client/src/Pages/Infrastructure/Create/index.jsx - URL construction logic
- Submitted PR Code:
url: `http${https ? "s" : ""}://` + infrastructureMonitor.url- Analysis:
- Current logic concatenates protocol with user input without validation
- Potential for double protocols (e.g.,
https://https://example.com) - No input sanitization which could lead to malformed URLs
- Business logic considerations: Could disrupt monitoring if invalid URLs are saved
- LlamaPReview Suggested Improvements:
// UTILITY FUNCTION (add to utils)
export const normalizeUrl = (input, useHttps) => {
// Remove existing protocols
const cleaned = input.replace(/^(https?:\/\/)/i, '');
return `http${useHttps ? 's' : ''}://${cleaned}`;
};
// IN COMPONENT
import { normalizeUrl } from 'path/to/utils';
url: normalizeUrl(infrastructureMonitor.url, https)- Improvement rationale:
- Prevents double protocols and malformed URLs
- Centralizes URL handling logic for consistency
- Business value: Ensures monitor functionality after updates
📁 server/src/validation/joi.js - Backend validation
- Submitted PR Code:
url: joi.string()- Analysis:
- Current validation accepts any string without format checking
- No restriction to HTTP/HTTPS protocols
- Security considerations: Could allow malicious URLs
- LlamaPReview Suggested Improvements:
url: joi.string().uri({
scheme: ['http', 'https'],
allowRelative: false
}).optional()- Improvement rationale:
- Ensures only valid HTTP/HTTPS URLs are accepted
- Maintains optionality for partial updates
- Security: Prevents potential SSRF vulnerabilities
📁 client/src/Hooks/monitorHooks.js - API communication
- Submitted PR Code:
updatedFields = {
// ...
url: monitor.url,
}- Analysis:
- Sends raw URL without validation
- No error handling for invalid URLs
- Could lead to API errors with malformed URLs
- LlamaPReview Suggested Improvements:
// ADD VALIDATION IN HOOK
const validateUrl = (url) => {
try {
new URL(url);
return true;
} catch {
return false;
}
};
// IN updateMonitor
if (monitor.url && !validateUrl(monitor.url)) {
throw new Error('Invalid URL format');
}- Improvement rationale:
- Provides early client-side validation
- Improves user feedback for invalid inputs
- Reduces invalid API requests
3.2 Key Quality Aspects
- System scalability considerations: Current implementation scales appropriately for expected load
- Performance bottlenecks and optimizations: No significant performance impacts identified
- Testing strategy and coverage: Critical need for additional test coverage of URL handling scenarios
- Documentation needs: API documentation should be updated to reflect new URL field in update operations
4. Overall Evaluation
- Technical assessment: Well-implemented solution that addresses core requirements but needs hardening for production
- Business impact: High value for users needing to update monitor URLs without losing history
- Risk evaluation: Medium risk due to URL handling vulnerabilities that need addressing
- Notable positive aspects and good practices: Clean implementation that maintains backward compatibility
- Implementation quality: Good overall quality with specific areas needing improvement
- Final recommendation: Approve after addressing P0 issues and adding basic test coverage
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
server/src/validation/joi.js (1)
183-191:editMonitorBodyValidationstill blockstypeupdates & allows invalid URLs
- The PR goal includes changing the monitor type, yet the schema only adds
url. Anytypefield in the PATCH payload will still be stripped out by Joi and never reach the service layer.urlis declared as justjoi.string(). IncreateMonitorBodyValidationwe enforce a URI (joi.string().required()); edits deserve the same.uri()safeguard, otherwiseeditcan injectnot a url.const editMonitorBodyValidation = joi.object({ + type: joi + .string() + .valid("http", "ping", "pagespeed", "docker", "hardware", "port"), - url: joi.string(), + url: joi.string().uri(),Let’s not be the stereotypical polite Canadian who apologises after the 500 error—stop it before it happens. 🤝
client/src/Hooks/monitorHooks.js (1)
351-366:typeis missing from the update payload
updatedFieldsnow carriesurl, but the PR also advertises “update monitor type”.
Without addingtype: monitor.type, the frontend can never request that change.const updatedFields = { name: monitor.name, @@ jsonPath: monitor.jsonPath, url: monitor.url, + type: monitor.type,Optional: wrap
url/typeinif (monitor.url)to avoid clobbering existing data when other editors omit the field.
Even our neighbours to the south wouldn’t drive on the wrong side of a PATCH route—let’s keep the payload tidy. 🇨🇦🇺🇸
🧹 Nitpick comments (2)
client/src/Pages/Infrastructure/Create/index.jsx (2)
470-475: Redundantdisabled={false}propPassing
disabled={false}adds no value—React only renders the attribute when the value is truthy.- disabled={false}Drop it to keep JSX leaner than an American’s black coffee.
477-497: Add ARIA labels to protocol toggle for accessibilityThe new HTTP/HTTPS
ButtonGrouplooks great, but screen-reader users won’t know what the buttons do.-<Button variant="group" filled={https.toString()} onClick={() => setHttps(true)}> +<Button + aria-label="Use HTTPS" + variant="group" + filled={https.toString()} + onClick={() => setHttps(true)} +>Repeat for the HTTP button. Let’s ensure our UI is friendlier than a Canadian in a snowstorm. 🍁
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Hooks/monitorHooks.js(1 hunks)client/src/Pages/Infrastructure/Create/index.jsx(2 hunks)server/src/validation/joi.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
Applied to files:
server/src/validation/joi.jsclient/src/Hooks/monitorHooks.jsclient/src/Pages/Infrastructure/Create/index.jsx
🧬 Code Graph Analysis (1)
client/src/Hooks/monitorHooks.js (1)
client/src/Pages/Uptime/Create/index.jsx (1)
monitor(69-78)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/src/Utils/url.js (1)
1-13: Add focused unit tests for normalization/validation edge casesCover: scheme stripping (http/https), protocol-relative (
//example.com), whitespace, localhost/IPv6, and disallow non-http(s) schemes. I can draft Jest tests if helpful.Example test cases to consider:
- normalizeUrl("example.com", true) → "https://example.com"
- normalizeUrl("HTTPS://ExAmPle.com/path", false) → "http://ExAmPle.com/path"
- normalizeUrl("//example.com", true) → "https://example.com"
- validateUrl("ftp://example.com") → false
- validateUrl(" https://localhost:3000 ") → true
- validateUrl("http://") → false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/src/Hooks/monitorHooks.js(3 hunks)client/src/Pages/Infrastructure/Create/index.jsx(3 hunks)client/src/Utils/url.js(1 hunks)server/src/validation/joi.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/validation/joi.js
- client/src/Hooks/monitorHooks.js
- client/src/Pages/Infrastructure/Create/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
client/src/Utils/url.js
Outdated
| } catch { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This is not needed, validation should be done by joi when the rest of the form is validated
| error={errors["url"] ? true : false} | ||
| helperText={errors["url"]} | ||
| disabled={!isCreate} | ||
| disabled={false} |
There was a problem hiding this comment.
This is not necessary, it can be removed entirely.
client/src/Hooks/monitorHooks.js
Outdated
| const updateMonitor = async ({ monitor, redirect }) => { | ||
| if (monitor.url && !validateUrl(monitor.url)) { | ||
| throw new Error("Invalid URL format"); | ||
| } |
There was a problem hiding this comment.
Validation should be handled by joi when the form is validated
There was a problem hiding this comment.
Thank you for the feedback!
The additional client-side URL validation was suggested by the automated review (LlamaPReview) as a way to prevent malformed URLs from being sent to the backend. However, I agree that it makes more sense to keep all validation centralized in the existing Joi schema for consistency and maintainability, as per project conventions.
I will remove the redundant validateUrl utility and the related checks from the hook and components, and rely solely on Joi for input validation.
Thanks for clarifying the preferred approach. Will push the updates shortly!
There was a problem hiding this comment.
Thank you for the feedback! The additional client-side URL validation was suggested by the automated review (LlamaPReview) as a way to prevent malformed URLs from being sent to the backend. However, I agree that it makes more sense to keep all validation centralized in the existing Joi schema for consistency and maintainability, as per project conventions.
I will remove the redundant validateUrl utility and the related checks from the hook and components, and rely solely on Joi for input validation. Thanks for clarifying the preferred approach. Will push the updates shortly!
Hey @Yasir-Rafique ,
To be clear, we validate input on both the frotnend and the backend, and we use Joi to do that in both places. We should definitely validate before submitting to the backend, but we should use Joi to do so.
If you inspect the component you will see that input valdation happens on individual field change in onChange and the entire form is validated on submission in onSubmit.
Please do make sure that validation is applied here, thank you!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/Utils/url.js (2)
3-3: Swallow all leading slashes to prevent accidentalhttp:////outcomesAs written, inputs like
http:////example.comor////example.comstill leave extra leading slashes after normalization (Line [3]). Switching to\/+removes one-or-more slashes and prevents duplicate slashes in the final URL.Apply this diff:
- const cleaned = raw.replace(/^(?:https?:)?\/\//i, ""); + const cleaned = raw.replace(/^(?:https?:)?\/+/i, "");
3-4: Optional guard: avoid emitting a bare scheme when input is emptyIf the user submits an empty string, this currently returns
http://orhttps://, which isn’t a valid URL and can be confusing in the UI. Consider short-circuiting to an empty string (or leave it to Joi if that’s the intended single-source-of-truth).Minimal change:
- const cleaned = raw.replace(/^(?:https?:)?\/\//i, ""); - return `http${useHttps ? "s" : ""}://${cleaned}`; + const cleaned = raw.replace(/^(?:https?:)?\/+/i, ""); + if (!cleaned) return ""; + return `http${useHttps ? "s" : ""}://${cleaned}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Hooks/monitorHooks.js(2 hunks)client/src/Pages/Infrastructure/Create/index.jsx(3 hunks)client/src/Utils/url.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/Hooks/monitorHooks.js
- client/src/Pages/Infrastructure/Create/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
🔇 Additional comments (1)
client/src/Utils/url.js (1)
1-5: Solid, minimal normalization; aligns with http/https-only flowNull-safe, trims input, and handles protocol-relative URLs. This is tidy—very Canadian. Americans might just YOLO the paste button.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/src/Validation/validation.js (2)
385-389: Clarify error copy and align with behaviorMessage says “hostname” but code also allows IPs; also doesn’t mention credentials.
"string.uri": "Please enter a valid URL starting with http:// or https://", - "string.invalidUrl": - "Only hostname (optional port) is allowed — no path, query, or fragment.", + "string.invalidUrl": + "Only host or IP (optional port) is allowed — no path, query, fragment, or credentials.",
373-379: Normalize the returned value for consistencyReturning a canonicalized value avoids storing variants like http://EXAMPLE.com, http://example.com/, etc. See suggested diff above (return
${u.protocol}//${u.host}).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Pages/Infrastructure/Create/index.jsx(5 hunks)client/src/Validation/validation.js(1 hunks)server/src/validation/joi.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/validation/joi.js
- client/src/Pages/Infrastructure/Create/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
🔇 Additional comments (1)
client/src/Validation/validation.js (1)
370-382: Great move: replaced brittle regex with standards-based URL parsingUsing the URL parser instead of a giant regex improves correctness, readability, and maintenance. Polite like a Canadian border agent, but stricter than an American HOA.
client/src/Validation/validation.js
Outdated
| if (!/^https?:\/\//i.test(value)) return helpers.error("string.uri"); | ||
| try { | ||
| const u = new URL(value); | ||
| const hasPath = u.pathname && u.pathname !== "/"; | ||
| const hasQuery = !!u.search; | ||
| const hasHash = !!u.hash; | ||
| if (hasPath || hasQuery || hasHash) { | ||
| return helpers.error("string.invalidUrl"); | ||
| } | ||
| return value; | ||
| } catch { | ||
| return helpers.error("string.uri"); | ||
| } | ||
|
|
||
| return value; | ||
| }) | ||
| .messages({ | ||
| "string.empty": "This field is required.", | ||
| "string.uri": "The URL you provided is not valid.", | ||
| "string.invalidUrl": "Please enter a valid URL with optional port", | ||
| "string.uri": "Please enter a valid URL starting with http:// or https://", | ||
| "string.invalidUrl": | ||
| "Only hostname (optional port) is allowed — no path, query, or fragment.", | ||
| }), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm client/server validation parity (paths, creds, ports)
Ensure server-side schemas match these client constraints to prevent UX mismatches (client rejects while server accepts, or vice versa).
🏁 Script executed:
#!/bin/bash
# Verify server-side URL constraints mirror client rules:
# - http/https only
# - no path/query/fragment
# - no credentials
# - port in 1..65535
# Locate joi validation files
fd -i -a 'joi.js'
# Inspect monitor body validations
rg -n -A5 -B5 'createMonitorBodyValidation|editMonitorBodyValidation'
# Inspect URL rules and custom validators
rg -n -A6 -B3 'url.*joi|Joi.*uri|Joi.*custom|credentials|username|password|pathname|search|hash|port'
# As a fallback, scan for any URI validations that might allow paths
rg -n -A3 -B3 'uri\('Length of output: 295827
Align server-side URL validation with client constraints
The server currently uses
.string()
.uri({ scheme: ["http", "https"] })which only enforces the protocol. It still allows paths, query strings, fragments, and credentials—unlike the client’s custom validator that forbids them. This will lead to confusing UX where the client rejects a URL the server accepts (or vice versa).
Please update the schema in server/src/validation/joi.js:
- createMonitorBodyValidation.url (around line 158)
- editMonitorBodyValidation.url (around line 188)
to mirror the client logic by disallowing:
• path (u.pathname !== "/")
• query (u.search)
• fragment (u.hash)
• credentials (u.username/u.password)
and still enforcing a valid port (1–65535).
You can use Joi’s URI options (e.g. allowQuery: false, allowFragments: false, allowCredentials: false) or plug in a custom validator akin to the client’s new URL(value) check.
Let’s keep Canada and the U.S. arguing over hockey, not URL formats! 😉
🤖 Prompt for AI Agents
In server/src/validation/joi.js around lines 158 and 188, update the
createMonitorBodyValidation.url and editMonitorBodyValidation.url schemas to
match the client-side URL validation by disallowing paths, queries, fragments,
and credentials. Use Joi's URI options such as allowQuery: false,
allowFragments: false, allowCredentials: false to enforce these restrictions,
and ensure the port is valid (1-65535). Alternatively, implement a custom
validator similar to the client’s new URL(value) check that verifies pathname is
"/", no search or hash exists, and username and password are empty.
client/src/Validation/validation.js
Outdated
| const u = new URL(value); | ||
| const hasPath = u.pathname && u.pathname !== "/"; | ||
| const hasQuery = !!u.search; | ||
| const hasHash = !!u.hash; | ||
| if (hasPath || hasQuery || hasHash) { | ||
| return helpers.error("string.invalidUrl"); | ||
| } | ||
| return value; | ||
| } catch { |
There was a problem hiding this comment.
Block credentials in URL and validate port range
Currently, URLs like http://user:pass@host and ports outside 1–65535 slip through. This contradicts “Only hostname (optional port)” and risks credential leakage.
Apply this diff to harden validation:
- const hasPath = u.pathname && u.pathname !== "/";
+ const hasPath = u.pathname !== "/";
const hasQuery = !!u.search;
const hasHash = !!u.hash;
+ const hasCreds = !!(u.username || u.password);
+ if (hasCreds) {
+ return helpers.error("string.invalidUrl");
+ }
+ if (u.port) {
+ const portNum = Number(u.port);
+ if (!Number.isInteger(portNum) || portNum < 1 || portNum > 65535) {
+ return helpers.error("string.invalidUrl");
+ }
+ }
if (hasPath || hasQuery || hasHash) {
return helpers.error("string.invalidUrl");
}
- return value;
+ // Normalize to canonical form: scheme://host[:port]
+ return `${u.protocol}//${u.host}`;📝 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.
| const u = new URL(value); | |
| const hasPath = u.pathname && u.pathname !== "/"; | |
| const hasQuery = !!u.search; | |
| const hasHash = !!u.hash; | |
| if (hasPath || hasQuery || hasHash) { | |
| return helpers.error("string.invalidUrl"); | |
| } | |
| return value; | |
| } catch { | |
| const u = new URL(value); | |
| const hasPath = u.pathname !== "/"; | |
| const hasQuery = !!u.search; | |
| const hasHash = !!u.hash; | |
| const hasCreds = !!(u.username || u.password); | |
| if (hasCreds) { | |
| return helpers.error("string.invalidUrl"); | |
| } | |
| if (u.port) { | |
| const portNum = Number(u.port); | |
| if (!Number.isInteger(portNum) || portNum < 1 || portNum > 65535) { | |
| return helpers.error("string.invalidUrl"); | |
| } | |
| } | |
| if (hasPath || hasQuery || hasHash) { | |
| return helpers.error("string.invalidUrl"); | |
| } | |
| // Normalize to canonical form: scheme://host[:port] | |
| return `${u.protocol}//${u.host}`; | |
| } catch { |
🤖 Prompt for AI Agents
In client/src/Validation/validation.js around lines 372 to 380, the URL
validation does not block URLs containing user credentials or ports outside the
valid range 1–65535, which violates the requirement to allow only hostname and
optional port. Update the validation logic to check if the URL includes username
or password and reject it, and also verify that if a port is specified, it falls
within the valid range; if not, return the validation error "string.invalidUrl".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/src/Validation/validation.js (1)
370-391: Confirm client/server URL validation parity (paths, creds, ports), and align policyGreat move to use
new URL()and to block credentials + invalid port ranges. However, current client logic allows paths, queries, and fragments. Past feedback called for host-only (optional port) and parity with server-side schemas.
- If product intent is host-only: add checks to reject
pathname !== "/",search, andhash, and consider normalizing toprotocol//host.- If intent is to allow path/query/fragment: ensure the server allows these and also rejects credentials and invalid ports to avoid UX mismatches.
Run this script to audit server-side schemas and confirm parity:
#!/bin/bash # Verify server-side URL rules align with client: # - Only http/https # - Decide: host-only vs allow path/query/fragment (consistency!) # - Block credentials # - Port in [1..65535] # Inspect where create/edit monitor bodies are validated rg -n -A5 -B5 'createMonitorBodyValidation|editMonitorBodyValidation' server | sed -n '1,200p' # Look for Joi URL/URI usages and custom validators rg -n -A6 -B3 'url.*Joi|Joi.*uri|Joi.*custom|credentials|username|password|pathname|search|hash|port' server # General scan for Joi uri() usage rg -n -A3 -B3 'uri\(' serverIf host-only is desired, apply something like:
- //Paths, query, and fragments are now allowed - return value; + const hasPath = u.pathname !== "/"; + const hasQuery = !!u.search; + const hasHash = !!u.hash; + if (hasPath || hasQuery || hasHash) { + return helpers.error("string.invalidUrl"); + } + // Optional: normalize to canonical host form + return `${u.protocol}//${u.host}`;Let’s keep cross-border disputes to hockey, not URL formats.
🧹 Nitpick comments (1)
client/src/Validation/validation.js (1)
395-397: i18n consistency check for validation messagesThese messages are human-readable strings, not i18n keys. Given the PR claims i18n support for visible strings, verify whether these bubble to the UI as-is. If so, consider switching to translation keys for consistency with other validators.
Happy to translate “Please enter a valid URL starting with http:// or https://” into i18n keys faster than an American orders a supersized coffee.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Validation/validation.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
🪛 Biome (2.1.2)
client/src/Validation/validation.js
[error] 188-188: Do not add then to an object.
(lint/suspicious/noThenProperty)
|
Hello @ajhollid @gorkemcetin I just updated the PR with the following requested changes:
One additional Change I made:
The reason for doing this was my previous PR where www.google.com/404 was disallowed Please let me know if this is ok. |
client/src/Validation/validation.js
Outdated
| "number.min": "Port must be at least 1.", | ||
| "number.max": "Port must be at most 65535.", | ||
| "any.required": "Port is required for port and game monitors.", | ||
| "any.required": "Port is required for port monitors.", |
There was a problem hiding this comment.
Why has "game" been removed here? Please revert this change
There was a problem hiding this comment.
Unintended changes from the pull during the sync!
| "any.required": "Game selection is required for game monitors.", | ||
| }), | ||
| otherwise: joi.string().allow(null, ""), | ||
| }), |
There was a problem hiding this comment.
Why has this been removed? Please revert
There was a problem hiding this comment.
Unintended changes from the pull during the sync!
|
HI @ajhollid |
Cool, thanks for the update, will review as soon as possible. |
| [name]: value, | ||
| }); | ||
|
|
||
| const adjustedValue = |
There was a problem hiding this comment.
I think the logic implemented here can be simplified. So many nested ternary become hard to read in the longer run and maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/Pages/Infrastructure/Create/index.jsx (2)
241-241: Remove duplicate URL construction.The URL is already constructed and assigned on line 178. This duplicate assignment on line 241 is redundant and should be removed.
- url: `http${https ? "s" : ""}://` + infrastructureMonitor.url,
265-279: Simplify the onChange validation logic.The current implementation with temporary variable assignment and early return can be simplified. Also, the validation should use
pageSchemafor consistency.const onChange = (event) => { const { value, name } = event.target; setInfrastructureMonitor({ ...infrastructureMonitor, [name]: value, }); - let adjustedValue = value; - - if (name === "url" && value) { - adjustedValue = `http${https ? "s" : ""}://${value}`; - } + const adjustedValue = name === "url" && value + ? `http${https ? "s" : ""}://${value}` + : value; - const { error } = infrastructureMonitorValidation.validate( + const { error } = pageSchema.validate( { [name]: adjustedValue }, { abortEarly: false } ); setErrors((prev) => ({ ...prev, ...(error ? { [name]: error.details[0].message } : { [name]: undefined }), })); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/src/Pages/Infrastructure/Create/index.jsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/Pages/Infrastructure/Create/index.jsx (2)
client/src/Validation/validation.js (1)
infrastructureMonitorValidation(373-423)client/src/Components/Inputs/FieldWrapper/index.jsx (1)
FieldWrapper(6-38)
🔇 Additional comments (4)
client/src/Pages/Infrastructure/Create/index.jsx (4)
109-111: LGTM! Clean schema fork for conditional validation.The implementation correctly makes the
urlfield required during creation and optional during editing, which aligns perfectly with the PR objectives of allowing URL updates.
203-203: Good update to use the page-specific schema.The validation now correctly uses
pageSchemainstead of the baseinfrastructureMonitorValidation, ensuring the URL field is properly validated based on the create/edit context.
486-506: Protocol toggle is now always visible - great improvement!The protocol toggle UI being always rendered (not just during creation) correctly implements the requirement to allow switching between HTTP and HTTPS when editing. This will help users maintain their monitoring history when services change protocols.
Speaking of protocols, here's a fun fact: The difference between HTTP and HTTPS is like the difference between a Canadian saying "sorry" (polite but open) and an American saying "sorry" (encrypted with layers of litigation protection)! 😄
484-484: URL field correctly enabled for editing.Removing the
disabled={!isCreate}constraint properly allows URL editing during configuration, which is the core feature of this PR. Users can now update their monitor URLs without losing historical data.
|
Hi all, |
|
@Yasir-Rafique can you please take off the conflicts? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
client/src/Pages/Infrastructure/Create/index.jsx (2)
79-90: Remove dead code: METRICS/METRIC_PREFIX/hasAlertError/getAlertError are unused here.These helpers and constants aren’t used in this file and also exist in CustomAlertsSection. Keeping duplicate, unused logic invites drift.
Apply this diff:
- const METRICS = ["cpu", "memory", "disk", "temperature"]; - const METRIC_PREFIX = "usage_"; - const MS_PER_MINUTE = 60000; - - const hasAlertError = (errors) => { - return Object.keys(errors).filter((k) => k.startsWith(METRIC_PREFIX)).length > 0; - }; - - const getAlertError = (errors) => { - const errorKey = Object.keys(errors).find((key) => key.startsWith(METRIC_PREFIX)); - return errorKey ? errors[errorKey] : null; - }; + const MS_PER_MINUTE = 60000;Bonus: somewhere a Canadian just shed a single polite tear for the loss of dead code. It’s okay—they’ll apologize to it on the way out.
191-210: onChange uses setErrors without defining it and duplicates hook logic.
setErrorsisn’t defined anywhere in this component; this will throw.- You’re bypassing your validation hook (
useValidateInfrastructureForm) and rebuilding per-field validation here, which risks drift and breaks encapsulation. A previous reviewer also flagged this pattern.Options to fix:
- Preferred: Delegate to your existing hook functions and remove the manual validation here. For example, use the hook’s
onChangeFormandvalidateFieldso every page benefits from consistent rules (and the URL scheme prefix can be handled there).- Minimal change: Destructure
setErrorsfromuseValidateInfrastructureFormand keep this code, but that still duplicates logic and is harder to maintain.If you stick with the hook, wire the inputs like:
// Use the hook-provided handler <TextInput ... onChange={onChangeForm} /> <CustomAlertsSection ... onChange={onChangeForm} />If you insist on local validation, remember to destructure:
const { errors, setErrors, validateField, validateForm } = useValidateInfrastructureForm();…and keep the schema source of truth single.
🧹 Nitpick comments (3)
client/src/Pages/Infrastructure/Create/index.jsx (3)
139-146: Form-level validation returns early but doesn’t surface errors to the UI.You validate and
returnon error, but nothing updateserrorsfor the user. That’s a UX dead-end.Use your hook’s
validateFormso errors are pushed into state, or map Joi errors yourself. For example:- const { error } = pageSchema.validate(form, { - abortEarly: false, - }); - - if (error) { - return; - } + if (!validateForm(form, pageSchema)) { + return; + }If
validateFormdoesn’t accept a schema override, either update the hook to accept one (recommended), or set errors fromerror.detailsbefore returning.
331-351: Re-validate URL when protocol is toggled.Toggling HTTP/HTTPS changes what the full URL will be, but no validation is triggered until the user types in the URL field. This can momentarily leave UI feedback out-of-sync.
Trigger a URL re-validation on toggle, e.g.:
onClick={() => { setHttps(true); // Option A: call validateField for 'url' using current value with https // Option B: force-blur/validate URL input or set a local effect reacting to `https` }}Small polish, but helps prevent confusion. Americans might argue “it’s fine,” but your QA friend from Toronto will raise an eyebrow.
408-415: Internationalization consistency: “Check frequency” should be translated.Most user-facing text uses i18n, but
label="Check frequency"is hardcoded.Consider:
label={t("checkFrequency")}Use whatever existing key matches your locale files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/src/Pages/Infrastructure/Create/index.jsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/Pages/Infrastructure/Create/index.jsx (5)
client/src/Pages/Infrastructure/Create/Components/CustomAlertsSection.jsx (4)
METRICS(17-17)METRIC_PREFIX(18-18)hasAlertError(19-21)getAlertError(22-25)client/src/Validation/validation.js (1)
infrastructureMonitorValidation(379-429)client/src/Pages/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-18)client/src/Hooks/monitorHooks.js (3)
error(412-412)createMonitor(305-318)updateMonitor(368-406)client/src/Pages/Infrastructure/Create/hooks/useInfrastructureSubmit.jsx (2)
createMonitor(3-3)updateMonitor(4-4)
🪛 Biome (2.1.2)
client/src/Pages/Infrastructure/Create/index.jsx
[error] 212-212: Shouldn't redeclare 'handleCheckboxChange'. Consider to delete it or rename it.
'handleCheckboxChange' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: Format Check (Client & Server)
client/src/Pages/Infrastructure/Create/index.jsx
[warning] 1-1: Code style issues found by Prettier in this file. Run 'prettier --write' to fix.
🔇 Additional comments (3)
client/src/Pages/Infrastructure/Create/index.jsx (3)
148-177: LGTM: Threshold normalization and payload shaping are clear.The normalization of usage_* to fractions and the conditional inclusion based on toggles are solid and readable. This aligns with the server’s expected shape.
1-437: Prettier flagged formatting; please run the formatter.CI reported Prettier issues for this file.
Run your project’s standard formatter (e.g.,
pnpm formatorprettier --write client/src/Pages/Infrastructure/Create/index.jsx) to keep diffs clean and CI happy.
96-106: No duplicate protocol risk in update initializationThe
initializeInfrastructureMonitorForUpdatefunction strips any leadinghttp://orhttps://frommonitor.url(.replace(/^https?:\/\//, "")), and you’re prepending the correct scheme on submit. That guarantees you won’t end up withhttp://http://…orhttps://https://….P.S. Even Canadians and Americans can agree: double protocols are about as welcome as double-doubles at Tim Hortons!
Yasir-Rafique
force-pushed
the
feat/edit-monitor-url-type
branch
2 times, most recently
from
a72f25d to
6e3c8b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/src/Pages/Infrastructure/Create/index.jsx (1)
92-94: Missing import for infrastructureMonitorValidation and pageSchema is unused. Pick one path.
- If you want to keep the forked schema, import it and wire it into your validation hook; otherwise it’s dead code.
- Easiest: remove pageSchema entirely to avoid the missing import and unused variable.
- const pageSchema = infrastructureMonitorValidation.fork(["url"], (s) => - isCreate ? s.required() : s.optional() - );
🧹 Nitpick comments (2)
server/src/validation/joi.js (1)
188-211: Align edit URL validation with create (add trim) and make gameId consistent.On edit:
- url lacks .trim(), unlike create. Trimming on both prevents subtle whitespace bugs.
- gameId allows empty string on create but not on edit. Aligning avoids failing when the client sends "" to clear it.
Apply:
const editMonitorBodyValidation = joi.object({ - url: joi - .string() - .uri({ scheme: ["http", "https"] }) - .optional(), + url: joi + .string() + .trim() + .uri({ scheme: ["http", "https"] }) + .optional(), name: joi.string(), description: joi.string(), interval: joi.number(), notifications: joi.array().items(joi.string()), secret: joi.string(), ignoreTlsErrors: joi.boolean(), jsonPath: joi.string().allow(""), expectedValue: joi.string().allow(""), matchMethod: joi.string().allow(null, ""), port: joi.number().min(1).max(65535), thresholds: joi.object().keys({ usage_cpu: joi.number(), usage_memory: joi.number(), usage_disk: joi.number(), usage_temperature: joi.number(), }), - gameId: joi.string(), + gameId: joi.string().allow(""), });client/src/Pages/Infrastructure/Create/index.jsx (1)
79-90: Remove unused constants/helpers (duplication with CustomAlertsSection and dead code).METRICS, METRIC_PREFIX, MS_PER_MINUTE, hasAlertError, and getAlertError aren’t used in this component. CustomAlertsSection already defines the alert helpers. Keep this page lean.
- const METRICS = ["cpu", "memory", "disk", "temperature"]; - const METRIC_PREFIX = "usage_"; - const MS_PER_MINUTE = 60000; - - const hasAlertError = (errors) => { - return Object.keys(errors).filter((k) => k.startsWith(METRIC_PREFIX)).length > 0; - }; - - const getAlertError = (errors) => { - const errorKey = Object.keys(errors).find((key) => key.startsWith(METRIC_PREFIX)); - return errorKey ? errors[errorKey] : null; - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
client/src/Hooks/monitorHooks.js(2 hunks)client/src/Pages/Infrastructure/Create/index.jsx(3 hunks)server/src/validation/joi.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/Hooks/monitorHooks.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/Pages/Infrastructure/Create/index.jsx (4)
client/src/Pages/Infrastructure/Create/Components/CustomAlertsSection.jsx (4)
METRICS(17-17)METRIC_PREFIX(18-18)hasAlertError(19-21)getAlertError(22-25)client/src/Pages/Uptime/Create/index.jsx (2)
MS_PER_MINUTE(106-106)errors(66-66)client/src/Validation/validation.js (1)
infrastructureMonitorValidation(379-429)client/src/Pages/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx (1)
infrastructureMonitor(3-18)
🪛 Biome (2.1.2)
client/src/Pages/Infrastructure/Create/index.jsx
[error] 135-135: Can't assign form because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (2)
server/src/validation/joi.js (1)
158-163: Good call tightening URL validation (http/https + trim).Aligns server-side validation with the client and PR objectives; permits paths/query/fragment while ensuring the scheme is correct.
client/src/Pages/Infrastructure/Create/index.jsx (1)
267-287: UI/UX win: protocol toggle available during edit.Making HTTP/HTTPS switchable in Configure aligns perfectly with the PR goal and saves users from delete/recreate churn. Nice—like finding a decent cup of coffee on either side of the border.
| const thresholds = { | ||
| ...(cpu ? { usage_cpu: usage_cpu / 100 } : {}), | ||
| ...(memory ? { usage_memory: usage_memory / 100 } : {}), | ||
| ...(disk ? { usage_disk: usage_disk / 100 } : {}), | ||
| ...(temperature ? { usage_temperature: usage_temperature / 100 } : {}), | ||
| }; | ||
|
|
||
| form = { | ||
| ...(isCreate ? {} : { _id: monitorId }), | ||
| ...rest, | ||
| url: `http${https ? "s" : ""}://` + infrastructureMonitor.url, | ||
| description: form.name, | ||
| type: "hardware", | ||
| notifications: infrastructureMonitor.notifications, | ||
| thresholds, | ||
| }; | ||
| submitInfrastructureForm(infrastructureMonitor, form, isCreate, monitorId); |
There was a problem hiding this comment.
Fix: const reassignment of form (compile error).
You reassign form after declaring it as const. Let's not trigger a cross-border incident between const and let—just build a separate payload.
- form = {
+ const payload = {
...(isCreate ? {} : { _id: monitorId }),
...rest,
url: `http${https ? "s" : ""}://` + infrastructureMonitor.url,
description: form.name,
type: "hardware",
notifications: infrastructureMonitor.notifications,
thresholds,
};
- submitInfrastructureForm(infrastructureMonitor, form, isCreate, monitorId);
+ submitInfrastructureForm(infrastructureMonitor, payload, isCreate, monitorId);Optional: to harden threshold math, coerce numbers and bound to [0, 1] when the toggle is on (prevents accidental NaN/negatives sneaking in).
📝 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.
| const thresholds = { | |
| ...(cpu ? { usage_cpu: usage_cpu / 100 } : {}), | |
| ...(memory ? { usage_memory: usage_memory / 100 } : {}), | |
| ...(disk ? { usage_disk: usage_disk / 100 } : {}), | |
| ...(temperature ? { usage_temperature: usage_temperature / 100 } : {}), | |
| }; | |
| form = { | |
| ...(isCreate ? {} : { _id: monitorId }), | |
| ...rest, | |
| url: `http${https ? "s" : ""}://` + infrastructureMonitor.url, | |
| description: form.name, | |
| type: "hardware", | |
| notifications: infrastructureMonitor.notifications, | |
| thresholds, | |
| }; | |
| submitInfrastructureForm(infrastructureMonitor, form, isCreate, monitorId); | |
| const thresholds = { | |
| ...(cpu ? { usage_cpu: usage_cpu / 100 } : {}), | |
| ...(memory ? { usage_memory: usage_memory / 100 } : {}), | |
| ...(disk ? { usage_disk: usage_disk / 100 } : {}), | |
| ...(temperature ? { usage_temperature: usage_temperature / 100 } : {}), | |
| }; | |
| const payload = { | |
| ...(isCreate ? {} : { _id: monitorId }), | |
| ...rest, | |
| url: `http${https ? "s" : ""}://` + infrastructureMonitor.url, | |
| description: form.name, | |
| type: "hardware", | |
| notifications: infrastructureMonitor.notifications, | |
| thresholds, | |
| }; | |
| submitInfrastructureForm(infrastructureMonitor, payload, isCreate, monitorId); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 135-135: Can't assign form because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🤖 Prompt for AI Agents
In client/src/Pages/Infrastructure/Create/index.jsx around lines 128 to 144, you
are reassigning the const variable form which causes a compile error; instead,
build a new payload object (e.g., payload or formPayload) that spreads the
existing rest values and adds the computed fields (_id when !isCreate, url,
description, type, notifications, thresholds) and pass that payload to
submitInfrastructureForm; also compute thresholds into a separate object using
Number(...) and clamp each value to the [0,1] range when the corresponding
toggle is true to avoid NaN/negatives.
@Br0wnHammer Conflicts are gone now! |
|
@Br0wnHammer @ajhollid |
|
Hello @Br0wnHammer @ajhollid |
Recording.2025-08-08.175452.mp4
Describe your changes
This PR implements the ability to update an existing monitor’s URL and monitoring type (e.g., switching between HTTP and HTTPS) via the Configure menu, without needing to delete and recreate the monitor. This allows users to retain monitoring history while adapting to service changes.
Write your issue number after "Fixes "
Fixes #2594
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
New Features
Improvements