-
Notifications
You must be signed in to change notification settings - Fork 186
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.
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
FE - Advanced Settings Page Validation and Error Handling #986
FE - Advanced Settings Page Validation and Error Handling #986
Conversation
- Add validation to advanced settings page
WalkthroughThe changes involve enhancements to validation and error handling across multiple components. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdvancedSettings
participant Validation
participant Store
User->>AdvancedSettings: Input settings
AdvancedSettings->>Validation: Validate input
Validation-->>AdvancedSettings: Return validation results
AdvancedSettings->>AdvancedSettings: Update error state
AdvancedSettings->>Store: Dispatch settings update if no errors
sequenceDiagram
participant User
participant CreateMaintenance
participant Validation
participant API
User->>CreateMaintenance: Submit maintenance data
CreateMaintenance->>Validation: Check for validation errors
Validation-->>CreateMaintenance: Return error status
CreateMaintenance->>API: Fetch monitors if no errors
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
Client/src/Validation/validation.js (1)
131-161
: Yo, this schema's got more layers than my mom's lasagna!The new
advancedSettingsValidation
schema looks solid overall, covering various system configuration fields. It's like you're cookin' up a storm in the kitchen of validation, and I'm diggin' the aroma! Just a couple of things to keep an eye on:
Some fields like
jwtTTL
anddbType
are required but don't have specific format validations. You might wanna consider addin' some extra seasoning to these fields to make sure they're exactly how you want 'em.The
logLevel
field is as open as my arms before a rap battle. Maybe consider addin' some allowed values to keep it tight?Keep it up, you're killin' it!
🧰 Tools
🪛 Biome
[error] 155-155: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Client/src/Pages/Maintenance/CreateMaintenance/index.jsx (3)
149-175
: Yo, this fetch game is tight, but we can make it tighter! 💪This new
fetchMonitors
flow is smoother than mom's spaghetti, for real. But yo, we could level up that error handling a bit. How 'bout we give the user more deets when things go south?Try this on for size:
} catch (error) { - createToast({ body: "Failed to fetch data" }); + createToast({ body: `Failed to fetch data: ${error.message}` }); logger.error("Failed to fetch monitors", error); }This way, the user gets the 411 on what went wrong, you feel me?
217-227
: Yo, this submit function's got mad flow now! 🎤Adding that
hasValidationErrors
check is like droppin' a sick beat before the main verse. It's gonna catch those wack inputs before they mess up our whole track, you know what I'm sayin'?But yo, let's make it crystal clear for the next MC who's gonna spit on this code. How 'bout we add a little comment to explain why we're bailin' if we got errors?
Try this remix:
const handleSubmit = async () => { - if(hasValidationErrors(form, maintenanceWindowValidation, setErrors)) + // Bail out if there are any validation errors + if (hasValidationErrors(form, maintenanceWindowValidation, setErrors)) return; // Build timestamp for maintenance window from startDate and startTime const start = dayjs(form.startDate)Now it's clear why we're bouncing if the validation ain't clean, you feel me?
Line range hint
1-577
: Yo, this whole track is straight fire now! 🔥🎧Listen up, fam! This CreateMaintenance component's got a whole new vibe, and it's bumpin'! We've got that error handling on lock with those new imports, that fetch game's tighter than ever, and that submit function's droppin' beats like it's nobody's business.
But yo, I got one more idea to make this joint platinum. How 'bout we add some real-time validation feedback? You know, like when you're freestylin' and the crowd's reactin' to every line?
Here's the deal: We could hook up those validation checks to run on every form change. That way, users get instant feedback, like a live audience, you feel me? It'd be like:
const handleFormChange = (key, value) => { setForm({ ...form, [key]: value }); validateField(key, value); }; const validateField = (key, value) => { const { error } = maintenanceWindowValidation.validate( { [key]: value }, { abortEarly: false } ); setErrors((prev) => buildErrors(prev, key, error)); };This way, our users are gonna feel like they're droppin' rhymes with a hype man backin' them up every step of the way! What you think? Wanna take this track to the next level?
Client/src/Pages/AdvancedSettings/index.jsx (1)
91-106
: Don't let console logs steal your thunderIn
handleSave
, you've got aconsole.log
lingering in line 99. Time to clean it up before the moment's gone.Remove the
console.log
statement to keep your production code clean.if (action.payload.success) { - console.log(action.payload.data); setLocalSettings(action.payload.data); body = "Settings saved successfully"; } else {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Client/src/Pages/AdvancedSettings/index.jsx (2 hunks)
- Client/src/Pages/Maintenance/CreateMaintenance/index.jsx (3 hunks)
- Client/src/Validation/error.js (1 hunks)
- Client/src/Validation/validation.js (1 hunks)
🧰 Additional context used
🪛 Biome
Client/src/Validation/validation.js
[error] 155-155: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (8)
Client/src/Validation/validation.js (1)
163-170
: You're exportin' more heat than my mixtape!The export statement is on point, includin' the new
advancedSettingsValidation
schema. It's like you're droppin' a fresh track in your validation album. Keep that fire comin'!Client/src/Pages/Maintenance/CreateMaintenance/index.jsx (1)
31-31
: Yo, these imports are fire! 🔥Bringin' in
buildErrors
andhasValidationErrors
is like addin' secret sauce to our spaghetti code. It's gonna make our error handlin' game strong, you feel me?Client/src/Pages/AdvancedSettings/index.jsx (6)
15-16
: Bringing in new imports to seize the momentYou've added
advancedSettingsValidation
,buildErrors
, andhasValidationErrors
to the mix. This sets the stage for enhanced validation—no issues here. Keep up the momentum!
20-24
: Guarding routes like there's everything to ownYour refactored
useEffect
checks if the user is an admin and navigates accordingly. Smooth move to prevent unauthorized access before it's too late.
30-42
: Initializing state to capture it allSetting up
localSettings
with default values ensures you're ready when opportunity comes knocking. No sweat here.
43-43
: Preparing to handle errors when they come heavyIntroducing
errors
state means you're ready to display validation feedback. You're on the right track to keep the code rock steady.
45-55
: Async calls wrapped tight, no letting it slipYour
useEffect
fetches settings without blocking the flow. DefininggetSettings
inside keeps the rhythm in control.
268-274
: Your save button's set to launch you to the topThe
LoadingButton
is ready to handle the save action. Just make sure the loading states are synced, so nothing stops.
type="text" | ||
id="systemEmailHost" | ||
label="Email host" | ||
name="systemEmailHost" | ||
value={localSettings.systemEmailHost} | ||
onChange={handleChange} | ||
error={errors.systemEmailHost} | ||
/> | ||
<Field | ||
type="number" | ||
id="systemEmailPort" | ||
label="System email address" | ||
name="systemEmailPort" | ||
value={localSettings.systemEmailPort.toString()} | ||
onChange={handleChange} | ||
error={errors.systemEmailPort} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind the labels so users don't drop
The Field
with id="systemEmailPort"
has a label of "System email address." This might cause some to lose their grip.
Ensure the label matches the field's purpose to avoid confusion.
<Field
type="number"
id="systemEmailPort"
- label="System email address"
+ label="System email port"
name="systemEmailPort"
value={localSettings.systemEmailPort.toString()}
onChange={handleChange}
error={errors.systemEmailPort}
/>
📝 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.
type="text" | |
id="systemEmailHost" | |
label="Email host" | |
name="systemEmailHost" | |
value={localSettings.systemEmailHost} | |
onChange={handleChange} | |
error={errors.systemEmailHost} | |
/> | |
<Field | |
type="number" | |
id="systemEmailPort" | |
label="System email address" | |
name="systemEmailPort" | |
value={localSettings.systemEmailPort.toString()} | |
onChange={handleChange} | |
error={errors.systemEmailPort} | |
/> | |
type="text" | |
id="systemEmailHost" | |
label="Email host" | |
name="systemEmailHost" | |
value={localSettings.systemEmailHost} | |
onChange={handleChange} | |
error={errors.systemEmailHost} | |
/> | |
<Field | |
type="number" | |
id="systemEmailPort" | |
label="System email port" | |
name="systemEmailPort" | |
value={localSettings.systemEmailPort.toString()} | |
onChange={handleChange} | |
error={errors.systemEmailPort} | |
/> |
const handleChange = (event) => { | ||
const { value, id } = event.target; | ||
setLocalSettings({ ...localSettings, [id]: value }); | ||
const { error } = advancedSettingsValidation.validate( | ||
{ [id]: value }, | ||
{ | ||
abortEarly: false, | ||
} | ||
); | ||
setErrors((prev) => { | ||
return buildErrors(prev, id, error); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validating on change so you don't miss your chance
In handleChange
, you're updating state and validating inputs on the fly. But be cautious not to let exceptions pass you by.
Consider wrapping your validation logic in a try-catch block to handle any unexpected validation errors gracefully.
const handleChange = (event) => {
const { value, id } = event.target;
setLocalSettings({ ...localSettings, [id]: value });
+ try {
const { error } = advancedSettingsValidation.validate(
{ [id]: value },
{
abortEarly: false,
}
);
setErrors((prev) => {
return buildErrors(prev, id, error);
});
+ } catch (err) {
+ console.error('Validation error:', err);
+ }
};
📝 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 handleChange = (event) => { | |
const { value, id } = event.target; | |
setLocalSettings({ ...localSettings, [id]: value }); | |
const { error } = advancedSettingsValidation.validate( | |
{ [id]: value }, | |
{ | |
abortEarly: false, | |
} | |
); | |
setErrors((prev) => { | |
return buildErrors(prev, id, error); | |
}); | |
}; | |
const handleChange = (event) => { | |
const { value, id } = event.target; | |
setLocalSettings({ ...localSettings, [id]: value }); | |
try { | |
const { error } = advancedSettingsValidation.validate( | |
{ [id]: value }, | |
{ | |
abortEarly: false, | |
} | |
); | |
setErrors((prev) => { | |
return buildErrors(prev, id, error); | |
}); | |
} catch (err) { | |
console.error('Validation error:', err); | |
} | |
}; |
<LoadingButton | ||
loading={settings.isLoading || settings.authIsLoading} | ||
variant="contained" | ||
color="primary" | ||
sx={{ px: theme.spacing(12), mt: theme.spacing(20) }} | ||
onClick={handleSave} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Seize every error before it's gone
You're not displaying validation errors for the LoadingButton
. Consider disabling it when there are validation errors, so users can't go wrong.
Disable the save button when there are validation errors to prevent invalid submissions.
<LoadingButton
loading={settings.isLoading || settings.authIsLoading}
variant="contained"
color="primary"
sx={{ px: theme.spacing(12), mt: theme.spacing(20) }}
onClick={handleSave}
+ disabled={Object.keys(errors).length > 0}
>
Save
</LoadingButton>
📝 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.
<LoadingButton | |
loading={settings.isLoading || settings.authIsLoading} | |
variant="contained" | |
color="primary" | |
sx={{ px: theme.spacing(12), mt: theme.spacing(20) }} | |
onClick={handleSave} | |
> | |
<LoadingButton | |
loading={settings.isLoading || settings.authIsLoading} | |
variant="contained" | |
color="primary" | |
sx={{ px: theme.spacing(12), mt: theme.spacing(20) }} | |
onClick={handleSave} | |
disabled={Object.keys(errors).length > 0} | |
> | |
Save | |
</LoadingButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Couple minor changes and we should be good to go, please see my comments in the code review!
Thanks for your hard work here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Validation/error.js (1 hunks)
- Client/src/Validation/validation.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Validation/error.js
🧰 Additional context used
🪛 Biome
Client/src/Validation/validation.js
[error] 155-155: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
redisPort: joi.string().allow('').custom((value, helpers) => { | ||
if(value && isNaN(parseInt(value))){ | ||
return helpers.message("Redit port must be a number") | ||
} | ||
return value | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo, this isNaN
check's got me more nervous than Eminem before a rap battle!
Listen up, homie! That isNaN()
you're using? It's shakier than my knees when I'm about to drop a sick beat. It tries to do some funky type coercion, which can lead to some whack results.
Let's flip this script and use Number.isNaN()
instead. It's tighter than my rhymes and won't let any sneaky values slip through. Here's how we gonna do it:
- redisPort: joi.string().allow('').custom((value, helpers) => {
- if(value && isNaN(parseInt(value))){
- return helpers.message("Redit port must be a number")
- }
- return value
- }),
+ redisPort: joi.string().allow('').custom((value, helpers) => {
+ if(value && Number.isNaN(Number(value))){
+ return helpers.message("Redis port must be a number")
+ }
+ return value
+ }),
Also, fixed that typo in "Redis". Can't have spelling errors in our lyrics, ya know?
📝 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.
redisPort: joi.string().allow('').custom((value, helpers) => { | |
if(value && isNaN(parseInt(value))){ | |
return helpers.message("Redit port must be a number") | |
} | |
return value | |
}), | |
redisPort: joi.string().allow('').custom((value, helpers) => { | |
if(value && Number.isNaN(Number(value))){ | |
return helpers.message("Redis port must be a number") | |
} | |
return value | |
}), |
🧰 Tools
🪛 Biome
[error] 155-155: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
const advancedSettingsValidation = joi.object({ | ||
apiBaseUrl: joi.string().uri({ allowRelative: true }).trim().messages({ | ||
"string.empty": "API base url is required.", | ||
"string.uri": "The URL you provided is not valid.", | ||
}), | ||
logLevel: joi.string().valid("debug", "none", "error", "warn").allow(""), | ||
systemEmailHost: joi.string().allow(''), | ||
systemEmailPort: joi.string().allow(''), | ||
systemEmailAddress: joi | ||
.string().allow(''), | ||
systemEmailPassword: joi.string().allow(''), | ||
jwtTTL: joi | ||
.string() | ||
.trim() | ||
.messages({ | ||
"string.empty": "JWT TTL is required." | ||
}), | ||
dbType: joi.string().trim().messages({ | ||
"string.empty": "DB type is required", | ||
}), | ||
redisHost: joi.string().trim().messages({ | ||
"string.empty": "Redis host is required", | ||
}), | ||
redisPort: joi.string().allow('').custom((value, helpers) => { | ||
if(value && isNaN(parseInt(value))){ | ||
return helpers.message("Redit port must be a number") | ||
} | ||
return value | ||
}), | ||
pagespeedApiKey: joi.string().allow('') | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Whoa, spot on catching that discordance!
Looks like our redisPort
is riding two different waves between client and backend:
- Client side: It's a
string
with customisNaN
validation. - Backend side: It's a
number
with.allow(null, "")
.
This mismatch might trip up validations down the line. Let's align them to keep the flow smooth:
- Update the client-side
redisPort
to be anumber
, removing theisNaN
check since Joi handles number validation.
🔗 Analysis chain
Yo, this schema's got more layers than my mom's lasagna!
The advancedSettingsValidation
schema's looking tight, covering all them advanced settings like a boss. But yo, I got a couple things that's making my palms sweaty:
- That
logLevel
validation? It's on point, matching them backend rules like they're spittin' the same rhyme. - Some fields are chill with empty strings, but others ain't. Make sure that's intentional, ya feel me?
Let's make sure we ain't missin' no beats. Run this to check if all the fields match what the backend's expectin':
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all fields in advancedSettingsValidation match backend expectations
# Test: Look for validation rules in the backend
rg -A 5 'logLevel:|apiBaseUrl:|systemEmailHost:|systemEmailPort:|systemEmailAddress:|systemEmailPassword:|jwtTTL:|dbType:|redisHost:|redisPort:|pagespeedApiKey:' ./Server/validation/joi.js
Length of output: 794
🧰 Tools
🪛 Biome
[error] 155-155: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like isNan
is still pressent, and there's a small typo: "Redit" instead of "Redis
I'll mark this approved as I'm good to go otherwise, if you can just resolve those last two we can merge in.
Add validation to advanced settings page
Below is the screen recording of the validation :
Screencast from 2024-10-18 14-07-59.webm