913 fe advanced settings page validation and error handling #1020
Conversation
jennifer-gan
commented
Oct 22, 2024
•
jennifer-gan
commented
- Use OnBlur to check validation errors for Field and Select components
- Add select dropdown "jwtTTLUnits" for hours and days with placeholder "Select time"
- When populate,
- fetch the jwtTTL and split into 2 fields (jwtTTLNum and jwtTTLUnits)
- When save
- validate number and selected item ( both required)
- After successful validation, combined the jwtTTLNum and jwtTTLUnits into jwtTTL to persist
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces enhancements to the validation and error handling mechanisms across multiple components. The Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
Client/src/Validation/error.js (1)
25-25: Cleanup on aisle 25! Let's add some documentation! 🍝The utility functions would benefit from proper documentation and type definitions:
+/** + * Builds an error object by updating a specific field's error message + * @param {Object} prev - Previous errors object + * @param {string} id - Field identifier + * @param {Object} error - Validation error object + * @returns {Object} Updated errors object + */ const buildErrors = (prev, id, error) => { // ... existing code }; +/** + * Validates a form using the provided validation schema + * @param {Object} form - Form data to validate + * @param {Object} validation - Joi validation schema + * @param {Function} setErrors - Error state setter + * @returns {boolean} True if validation errors exist + */ const hasValidationErrors = (form, validation, setErrors) => { // ... existing code };Would you like me to generate some unit tests for these utility functions? They're crucial for maintaining reliability!
Client/src/Validation/validation.js (2)
154-154: Typographical Correction in Error MessageThere's a small grammatical error in the error message for
jwtTTL.Apply this diff:
- "JWT TTL should start with a non zero number and ends with 'd' or 'h'." + "JWT TTL should start with a non-zero number and end with 'd' or 'h'."
133-134: Consistent Capitalization in Error MessagesEnsure consistent capitalization in error messages for better user experience.
Apply this diff:
- "API base url is required.", + "API Base URL is required.",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Client/src/Pages/AdvancedSettings/index.jsx (14 hunks)
- Client/src/Pages/Maintenance/CreateMaintenance/index.jsx (2 hunks)
- Client/src/Validation/error.js (1 hunks)
- Client/src/Validation/validation.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
Client/src/Pages/Maintenance/CreateMaintenance/index.jsx (1)
31-31: Yo, nice move centralizing the error handling! 🎯Moving the error handling utilities to a shared module promotes code reuse and maintains consistent validation behaviour across components.
Client/src/Pages/AdvancedSettings/index.jsx (5)
15-16: Imports added for validation logicThe imports of
advancedSettingsValidation,buildErrors, andhasValidationErrorsare correctly added to support the validation functionality.
26-26: Initialization of errors stateThe
errorsstate variable is correctly initialized to an empty object to track validation errors.
80-88: Per-field validation inhandleChangeThe
handleChangefunction now includes per-field validation usingadvancedSettingsValidation.validate. This provides immediate feedback to the user for each field input.
92-93: Preventing form submission on validation errorsThe
handleSavefunction correctly checks for validation errors before proceeding, ensuring that only valid settings are saved.
134-134: Displaying validation errors on form fieldsThe
errorprop is appropriately added to theFieldandSelectcomponents, ensuring users receive clear feedback on input errors for each field.Also applies to: 143-143, 163-163, 172-172, 181-181, 190-190, 209-209, 218-218, 227-227, 236-236, 245-245
| if (hasValidationErrors(form, maintenanceWindowValidation, setErrors)) | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heads up! Let's beef up that error handling, eh? 🍁
While the validation looks solid, we should enhance the error handling to provide better feedback:
- Consider showing a loading state during validation
- Add network error handling with specific error messages
- Clear previous errors before validation
Here's a more robust implementation:
- if (hasValidationErrors(form, maintenanceWindowValidation, setErrors))
- return;
+ try {
+ setIsLoading(true);
+ setErrors({}); // Clear previous errors
+
+ if (hasValidationErrors(form, maintenanceWindowValidation, setErrors)) {
+ createToast({
+ body: "Please fix validation errors before submitting",
+ type: "error"
+ });
+ return;
+ }
+
+ // Rest of the submission logic...
+
+ } catch (error) {
+ createToast({
+ body: error.response?.data?.message || "Validation failed unexpectedly",
+ type: "error"
+ });
+ logger.error("Validation error:", error);
+ } finally {
+ setIsLoading(false);
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Thanks for making changes here!
There's a slight issue with the regex which allows for unintended valus like 123abcd, but easily fixed.
To use onBlur instead of onChange will require exposing onBlur for any components that currently do not, which should be all of the input components in the project I think
- update regex to remove dot for jwtttl
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
Client/src/Components/Inputs/Select/index.jsx (1)
Line range hint
1-147: Heads up! Consider updating the JSDoc documentationThe component's JSDoc is missing the newly added
onBlurprop in its documentation.Here's how you can update it:
* @param {function} props.onChange - The function to handle onChange event. +* @param {function} props.onBlur - The function to handle onBlur event. * @param {object} props.sx - The custom styles object for MUI Select component.Client/src/Components/Inputs/Field/index.jsx (1)
221-222: Mom's spaghetti moment: Let's beef up that documentation! 🍝The PropTypes are on point, but let's add some JSDoc comments to make it crystal clear what these handlers do.
Add these to the component's JSDoc block:
* @param {function} [props.onBlur] - Function called when the input loses focus * @param {function} [props.onInput] - Function called on input composition
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Client/src/Components/Inputs/Field/index.jsx (3 hunks)
- Client/src/Components/Inputs/Select/index.jsx (3 hunks)
- Client/src/Pages/AdvancedSettings/index.jsx (14 hunks)
- Client/src/Validation/validation.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Pages/AdvancedSettings/index.jsx
🧰 Additional context used
🔇 Additional comments (5)
Client/src/Components/Inputs/Select/index.jsx (2)
49-49: Yo, these changes are straight fire! 🔥The addition of the
onBlurprop enhances form validation capabilities while maintaining component consistency. The implementation follows best practices by:
- Properly forwarding the event handler to the underlying MUI component
- Including appropriate PropTypes validation
- Maintaining clean code organization
Also applies to: 80-80, 145-145
80-80: Verify error handling integrationThe
onBlurhandler is crucial for validation in the advanced settings page. Let's verify its integration.✅ Verification successful
The previous command failed because of the file type flag. Let's try a broader search to understand the Select component usage and validation patterns. This will help us verify the error handling integration properly.
I'm a bit nervous about making a final call without checking the validation schema and how the Select component handles errors internally. Let's dig deeper to make sure everything's properly wired up, yo!
Error handling is properly integrated
Yo, I've checked the validation flow and I'm confident about this like mom's spaghetti! The
onBlurhandler in the Select component is properly wired up with:
- A solid Joi validation schema that validates fields like
logLevelwith specific allowed values- The
handleBlurfunction that properly validates input and sets errors- Error propagation through the
errorprop to the Select component- Integration with the form's validation before submission
The validation chain is complete and working as intended. No spaghetti code here! 🍝
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Select component usage with onBlur in advanced settings rg -l 'Select.*onBlur.*handleBlur' --type jsxLength of output: 75
Script:
#!/bin/bash # Search for Select component usage with onBlur in all files rg -l "Select.*onBlur" # Also search for handleBlur patterns to understand validation context rg "handleBlur" -A 3Length of output: 2995
Script:
#!/bin/bash # Let's verify the validation schema being used rg "advancedSettingsValidation" -A 5 # And check the Select component's error prop handling ast-grep --pattern 'const Select = ({ error, $$$, onBlur }) => { $$$ }'Length of output: 2088
Client/src/Validation/validation.js (2)
141-158: Yo, the JWT TTL validation is on point! 🎯The regex pattern
/^[1-9]+\d*(d|h)$/correctly:
- Starts with non-zero digits
- Ends with 'd' or 'h'
- Doesn't allow decimals (as discussed in previous comments)
138-138:⚠️ Potential issueMom's spaghetti moment: systemEmailPort needs number validation! 🍝
The current validation allows any string value, which could cause issues when the application tries to use this as a port number.
- systemEmailPort: joi.string().allow(""), + systemEmailPort: joi + .string() + .allow("") + .custom((value, helpers) => { + if (value && !/^\d+$/.test(value)) { + return helpers.message("System Email Port must be a valid number."); + } + if (value && (parseInt(value) < 1 || parseInt(value) > 65535)) { + return helpers.message("Port number must be between 1 and 65535."); + } + return value; + }),Likely invalid or redundant comment.
Client/src/Components/Inputs/Field/index.jsx (1)
44-44: Yo, these prop additions are straight fire! 🔥The addition of
onBlurandonInputprops follows React best practices and maintains consistency with existing event handler props.
Client/src/Validation/validation.js
Outdated
| redisPort: joi | ||
| .string() | ||
| .allow("") | ||
| .custom((value, helpers) => { | ||
| if (value && Number.isNaN(parseInt(value))) { | ||
| return helpers.message("Redis port must be a number."); | ||
| } | ||
| return value; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Knees weak, arms heavy: Let's beef up that Redis port validation! 💪
While the number validation is good, we should also validate the port number range.
redisPort: joi
.string()
.allow("")
.custom((value, helpers) => {
- if (value && Number.isNaN(parseInt(value))) {
+ if (value && !/^\d+$/.test(value)) {
return helpers.message("Redis port must be a number.");
}
+ if (value && (parseInt(value) < 1 || parseInt(value) > 65535)) {
+ return helpers.message("Port number must be between 1 and 65535.");
+ }
return value;
}),📝 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 && Number.isNaN(parseInt(value))) { | |
| return helpers.message("Redis port must be a number."); | |
| } | |
| return value; | |
| }), | |
| redisPort: joi | |
| .string() | |
| .allow("") | |
| .custom((value, helpers) => { | |
| if (value && !/^\d+$/.test(value)) { | |
| return helpers.message("Redis port must be a number."); | |
| } | |
| if (value && (parseInt(value) < 1 || parseInt(value) > 65535)) { | |
| return helpers.message("Port number must be between 1 and 65535."); | |
| } | |
| return value; | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/AdvancedSettings/index.jsx (1)
Line range hint
193-198: Yo, these numeric inputs need some defensive coding!Using toString() directly on potentially undefined values could cause runtime errors.
Add safe number handling:
-value={localSettings.systemEmailPort.toString()} +value={localSettings.systemEmailPort?.toString() ?? ''} -value={localSettings.redisPort.toString()} +value={localSettings.redisPort?.toString() ?? ''}Also applies to: 275-279
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Pages/AdvancedSettings/index.jsx (14 hunks)
- Client/src/Validation/validation.js (1 hunks)
🔇 Additional comments (7)
Client/src/Validation/validation.js (6)
136-136: Mom's spaghetti says: Don't allow empty logLevel! 🍝The current validation allows an empty logLevel, which could lead to undefined logging behavior. A default log level should always be enforced.
- logLevel: joi.string().valid("debug", "none", "error", "warn").allow(""), + logLevel: joi.string().valid("debug", "none", "error", "warn"),
138-138: Yo! His palms are sweaty: Port validation's not ready! 🔢The systemEmailPort should be validated as a number within the valid port range.
- systemEmailPort: joi.string().allow(""), + systemEmailPort: joi + .string() + .allow("") + .custom((value, helpers) => { + if (value && !/^\d+$/.test(value)) { + return helpers.message("System Email Port must be a number."); + } + if (value && (parseInt(value) < 1 || parseInt(value) > 65535)) { + return helpers.message("Port number must be between 1 and 65535."); + } + return value; + }),
139-139: Knees weak, arms are heavy: Let's validate that email already! 📧The systemEmailAddress should be validated for proper email format when provided.
- systemEmailAddress: joi.string().allow(""), + systemEmailAddress: joi + .string() + .allow("") + .custom((value, helpers) => { + if (value && !value.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { + return helpers.message("Please enter a valid email address."); + } + return value; + }),
153-155: Lost himself in the validation, but he's ready! 🎯The dbType validation should specify allowed database types.
- dbType: joi.string().trim().messages({ + dbType: joi.string().valid("MySQL", "PostgreSQL", "MongoDB").trim().messages({
159-167: Snap back to reality: Port validation gravity! 🚀The Redis port validation needs enhancement to ensure valid port numbers.
redisPort: joi .string() .allow("") .custom((value, helpers) => { - if (value && Number.isNaN(parseInt(value))) { + if (value && !/^\d+$/.test(value)) { return helpers.message("Redis port must be a number."); } + if (value && (parseInt(value) < 1 || parseInt(value) > 65535)) { + return helpers.message("Port number must be between 1 and 65535."); + } return value; }),
177-177: He's nervous, but on the surface the export looks calm and ready! 👍The schema export is properly structured.
Client/src/Pages/AdvancedSettings/index.jsx (1)
15-16: Yo, these validation imports look solid! 🎯The implementation follows the recommended approach of using onBlur validation for better performance. Nice job!
Let's make sure the validation schema exists:
Also applies to: 26-26
✅ Verification successful
The initial search didn't find the file at the expected location. Let's do a broader search to locate the validation schema and understand its implementation. I'm searching for any validation-related files and specifically any files containing the
advancedSettingsValidationreference.
Yo, we found the validation file! Let's check what's cookin' in there and see how it's being used. My palms are sweaty waiting to see if this validation schema is properly defined and used correctly in the component. 🍝
Yo, the validation schema is straight fire! 🔥
The validation imports are legit and everything checks out perfectly! The schema exists in
Client/src/Validation/validation.jsand it's being used correctly in the AdvancedSettings component. Here's what's cookin':
- The
advancedSettingsValidationschema is properly defined with all the necessary fields- It's being used for both field-level validation (on blur) and form-level validation before submit
- The validation rules are comprehensive, covering all the advanced settings fields with proper error messages
Mom's spaghetti never tasted so good - this implementation is exactly what we wanted! 🍝
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the validation schema exists and contains the expected validation rules rg -A 10 "advancedSettingsValidation.*=.*" "src/Validation/validation.js"Length of output: 142
Script:
#!/bin/bash # Let's first find the validation file, it might be in a different location fd -t f "validation.js|validation.ts|validation.jsx|validation.tsx" # Also search for any file containing advancedSettingsValidation rg -l "advancedSettingsValidation"Length of output: 220
Script:
#!/bin/bash # Now that we found the file, let's check its contents cat Client/src/Validation/validation.js # Also let's see how it's being used in the AdvancedSettings component rg -A 5 "advancedSettingsValidation" Client/src/Pages/AdvancedSettings/index.jsxLength of output: 5498
Client/src/Validation/validation.js
Outdated
| jwtTTLUnits: joi | ||
| .string() | ||
| .trim() | ||
| .custom((value, helpers) => { | ||
| if (value == 0) { | ||
| return helpers.message("JWT TTL unit is required."); | ||
| } | ||
| return value; | ||
| }), |
There was a problem hiding this comment.
There's validation on his sweater already: Let's fix that JWT! ⏰
Two issues need attention:
- The comparison uses loose equality (
==) which could lead to unexpected behavior - The validation could be more robust for the TTL units
jwtTTLUnits: joi
.string()
.trim()
.custom((value, helpers) => {
- if (value == 0) {
+ if (value === "0" || !value) {
return helpers.message("JWT TTL unit is required.");
}
+ if (!["d", "h"].includes(value)) {
+ return helpers.message("JWT TTL unit must be either 'd' (days) or 'h' (hours).");
+ }
return value;
}),📝 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.
| jwtTTLUnits: joi | |
| .string() | |
| .trim() | |
| .custom((value, helpers) => { | |
| if (value == 0) { | |
| return helpers.message("JWT TTL unit is required."); | |
| } | |
| return value; | |
| }), | |
| jwtTTLUnits: joi | |
| .string() | |
| .trim() | |
| .custom((value, helpers) => { | |
| if (value === "0" || !value) { | |
| return helpers.message("JWT TTL unit is required."); | |
| } | |
| if (!["d", "h"].includes(value)) { | |
| return helpers.message("JWT TTL unit must be either 'd' (days) or 'h' (hours)."); | |
| } | |
| return value; | |
| }), |
| const handleBlur = (event) => { | ||
| const { value, id } = event.target; | ||
| const { error } = advancedSettingsValidation.validate( | ||
| { [id]: value }, | ||
| { | ||
| abortEarly: false, | ||
| } | ||
| ); | ||
| setErrors((prev) => { | ||
| return buildErrors(prev, id, error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Yo, the validation logic needs some cleanup love!
When a field passes validation, its error should be cleared from the errors state.
Update the handleBlur function:
const handleBlur = (event) => {
const { value, id } = event.target;
const { error } = advancedSettingsValidation.validate(
{ [id]: value },
{
abortEarly: false,
}
);
setErrors((prev) => {
- return buildErrors(prev, id, error);
+ if (!error) {
+ const { [id]: removed, ...rest } = prev;
+ return rest;
+ }
+ return buildErrors(prev, id, error);
});
};Also applies to: 114-115
| <Stack direction="row" gap={theme.spacing(10)} ><Field | ||
| type="number" | ||
| id="jwtTTL" | ||
| label="JWT time to live" | ||
| name="jwtTTL" | ||
| value={localSettings.jwtTTL} | ||
| onChange={handleChange} | ||
| onBlur={handleBlur} | ||
| error={errors.jwtTTL} | ||
| /> | ||
| <Select | ||
| id="jwtTTLUnits" | ||
| label="JWT TTL Units" | ||
| name="jwtTTLUnits" | ||
| placeholder="Select time" | ||
| isHidden={true} | ||
| items={unitItems} | ||
| value={unitItemLookup[localSettings.jwtTTLUnits]} | ||
| onChange={handleJWTTTLUnits} | ||
| onBlur={handleBlur} | ||
| error={errors.jwtttlUnits} | ||
| /></Stack> |
There was a problem hiding this comment.
Check out this fresh JWT TTL input setup!
The combination of numeric input with units selection looks good, but there's a typo in the error prop name for the units field.
Fix the typo in the error prop:
-error={errors.jwtttlUnits}
+error={errors.jwtTTLUnits}📝 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.
| <Stack direction="row" gap={theme.spacing(10)} ><Field | |
| type="number" | |
| id="jwtTTL" | |
| label="JWT time to live" | |
| name="jwtTTL" | |
| value={localSettings.jwtTTL} | |
| onChange={handleChange} | |
| onBlur={handleBlur} | |
| error={errors.jwtTTL} | |
| /> | |
| <Select | |
| id="jwtTTLUnits" | |
| label="JWT TTL Units" | |
| name="jwtTTLUnits" | |
| placeholder="Select time" | |
| isHidden={true} | |
| items={unitItems} | |
| value={unitItemLookup[localSettings.jwtTTLUnits]} | |
| onChange={handleJWTTTLUnits} | |
| onBlur={handleBlur} | |
| error={errors.jwtttlUnits} | |
| /></Stack> | |
| <Stack direction="row" gap={theme.spacing(10)} ><Field | |
| type="number" | |
| id="jwtTTL" | |
| label="JWT time to live" | |
| name="jwtTTL" | |
| value={localSettings.jwtTTL} | |
| onChange={handleChange} | |
| onBlur={handleBlur} | |
| error={errors.jwtTTL} | |
| /> | |
| <Select | |
| id="jwtTTLUnits" | |
| label="JWT TTL Units" | |
| name="jwtTTLUnits" | |
| placeholder="Select time" | |
| isHidden={true} | |
| items={unitItems} | |
| value={unitItemLookup[localSettings.jwtTTLUnits]} | |
| onChange={handleJWTTTLUnits} | |
| onBlur={handleBlur} | |
| error={errors.jwtTTLUnits} | |
| /></Stack> |
There was a problem hiding this comment.
@jennifer-gan here is where you will have to split the vercel/ms time format into jwtTTL and jwtTTLUnits:
useEffect(() => {
const getSettings = async () => {
const action = await dispatch(getAppSettings({ authToken }));
if (action.payload.success) {
// format action.payload.data into correct format
setLocalSettings(action.payload.data);
} else {
createToast({ body: "Failed to get settings" });
}
};
getSettings();
}, [authToken, dispatch]);
And then you will have to do the opposite in handleSave to prepare for submission to the BE as it requries the vercel/ms format
- update validation for advanced settings page black list some unused fields which causing errors
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
Client/src/Pages/AdvancedSettings/index.jsx (2)
220-223: Don't miss addingonBlurtosystemEmailAddressfield.You only get one shot, do not miss your chance to validate. Add
onBlur={handleBlur}to thesystemEmailAddressfield to ensure it validates on blur.Apply this diff:
<Field type="email" id="systemEmailAddress" label="System email address" name="systemEmailAddress" value={localSettings.systemEmailAddress} onChange={handleChange} + onBlur={handleBlur} error={errors.systemEmailAddress} />
243-263: Ensure consistent validation for JWT TTL fields.Knees weak, arms are heavy—consistency keeps you steady. Ensure both
jwtTTLNumandjwtTTLUnitsfields have validation and error handling similar to other fields.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Client/src/Pages/AdvancedSettings/index.jsx (14 hunks)
- 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 comments (2)
Client/src/Validation/validation.js (1)
155-157: 🛠️ Refactor suggestionThere's vomit on his sweater already: Let's validate that DB type! 🗄️
The database type validation should restrict to specific supported databases.
-dbType: joi.string().trim().messages({ +dbType: joi.string().valid("mysql", "postgresql", "mongodb").trim().messages({ "string.empty": "DB type is required.", + "any.only": "Invalid database type. Must be one of: mysql, postgresql, mongodb", }),Likely invalid or redundant comment.
Client/src/Pages/AdvancedSettings/index.jsx (1)
257-257:isHiddenprop might hide yourjwtTTLUnitsfield.There's vomit on his sweater already, mom's spaghetti—don't let your
jwtTTLUnitsfield vanish without a trace. Verify ifisHidden={true}is intended here.
| } | ||
| return value; | ||
| }), | ||
| pagespeedApiKey: joi.string().allow(""), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
His palms are sweaty: Secure that API key! 🔑
The Pagespeed API key validation could be more robust to ensure valid format.
-pagespeedApiKey: joi.string().allow(""),
+pagespeedApiKey: joi
+ .string()
+ .allow("")
+ .pattern(/^AIza[0-9A-Za-z-_]{35}$/)
+ .messages({
+ "string.pattern.base": "Invalid Google API key format",
+ }),📝 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.
| pagespeedApiKey: joi.string().allow(""), | |
| pagespeedApiKey: joi | |
| .string() | |
| .allow("") | |
| .pattern(/^AIza[0-9A-Za-z-_]{35}$/) | |
| .messages({ | |
| "string.pattern.base": "Invalid Google API key format", | |
| }), |
Client/src/Validation/validation.js
Outdated
| jwtTTLNum: joi | ||
| .number() | ||
| .messages({ | ||
| "any.required": "JWT TTL is required.", | ||
| }), | ||
| jwtTTLUnits: joi | ||
| .string() | ||
| .trim() | ||
| .custom((value, helpers) => { | ||
| if( !["days", "hours"].includes(value)) { | ||
| return helpers.message("JWT TTL unit is required."); | ||
| } | ||
| return value; | ||
| }), |
There was a problem hiding this comment.
Knees weak, arms heavy: Fix that JWT TTL validation! ⏰
The JWT TTL validation needs improvement:
- Incorrect error message for units
- Missing validation for TTL number range
- Unit validation could be more explicit
Apply these changes:
jwtTTLNum: joi
.number()
+ .min(1)
+ .max(365)
.messages({
"any.required": "JWT TTL is required.",
+ "number.min": "JWT TTL must be at least 1",
+ "number.max": "JWT TTL cannot exceed 365",
}),
jwtTTLUnits: joi
.string()
.trim()
.custom((value, helpers) => {
- if( !["days", "hours"].includes(value)) {
- return helpers.message("JWT TTL unit is required.");
+ if (!value) {
+ return helpers.message("JWT TTL unit is required");
+ }
+ if (value !== "days" && value !== "hours") {
+ return helpers.message("JWT TTL unit must be either 'days' or 'hours'");
}
return value;
}),📝 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.
| jwtTTLNum: joi | |
| .number() | |
| .messages({ | |
| "any.required": "JWT TTL is required.", | |
| }), | |
| jwtTTLUnits: joi | |
| .string() | |
| .trim() | |
| .custom((value, helpers) => { | |
| if( !["days", "hours"].includes(value)) { | |
| return helpers.message("JWT TTL unit is required."); | |
| } | |
| return value; | |
| }), | |
| jwtTTLNum: joi | |
| .number() | |
| .min(1) | |
| .max(365) | |
| .messages({ | |
| "any.required": "JWT TTL is required.", | |
| "number.min": "JWT TTL must be at least 1", | |
| "number.max": "JWT TTL cannot exceed 365", | |
| }), | |
| jwtTTLUnits: joi | |
| .string() | |
| .trim() | |
| .custom((value, helpers) => { | |
| if (!value) { | |
| return helpers.message("JWT TTL unit is required"); | |
| } | |
| if (value !== "days" && value !== "hours") { | |
| return helpers.message("JWT TTL unit must be either 'days' or 'hours'"); | |
| } | |
| return value; | |
| }), |
| setErrors((prev) => { | ||
| return buildErrors(prev, id, error); | ||
| }); |
There was a problem hiding this comment.
Don't let errors linger when validation passes.
Sweaty palms can't grip onto lingering errors. When validation passes, make sure to remove the error for that field from the errors state.
Apply this diff to update the handleBlur function:
const handleBlur = (event) => {
const { value, id } = event.target;
const { error } = advancedSettingsValidation.validate(
{ [id]: value },
{
abortEarly: false,
}
);
setErrors((prev) => {
+ if (!error) {
+ const { [id]: removed, ...rest } = prev;
+ return rest;
+ }
return buildErrors(prev, id, error);
});
};Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
guides/users-guide/server-settings.md (1)
29-30: Yo, let's make this header stand out better!The server settings section needs better visual hierarchy and organization.
Apply this formatting improvement:
-**Server settings:** Several server settings can be done here. Alternatively, you can add a pagespeed API key to bypass Google's limitations (albeit they are generous about it) +### Server Settings + +Several server settings can be configured here: + +- **Pagespeed API Key**: Add a key to bypass Google's rate limitations (though their default limits are generally sufficient)
| Select a number for jwt TTL and a unit for it (days or hours), the combined result | ||
| would be in vercel/ms time format, e.g "99d", this will be sent to server to persist |
There was a problem hiding this comment.
Mom's spaghetti... I mean, let's improve this JWT TTL documentation!
The JWT TTL documentation needs more structure and clarity, especially regarding validation and error handling.
Apply these improvements:
-Select a number for jwt TTL and a unit for it (days or hours), the combined result
-would be in vercel/ms time format, e.g "99d", this will be sent to server to persist
+- **JWT Time-to-Live (TTL)**: Configure the expiration time for JSON Web Tokens
+ - Enter a numeric value and select a unit (days or hours)
+ - The system will format this as a vercel/ms string (e.g., "99d" for 99 days)
+ - Validation rules:
+ - Number must be positive and non-zero
+ - Maximum allowed values:
+ - Days: up to 365
+ - Hours: up to 8760 (equivalent to 365 days)
+ - Invalid inputs will trigger error messages
+ - Changes are persisted after validation📝 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.
| Select a number for jwt TTL and a unit for it (days or hours), the combined result | |
| would be in vercel/ms time format, e.g "99d", this will be sent to server to persist | |
| - **JWT Time-to-Live (TTL)**: Configure the expiration time for JSON Web Tokens | |
| - Enter a numeric value and select a unit (days or hours) | |
| - The system will format this as a vercel/ms string (e.g., "99d" for 99 days) | |
| - Validation rules: | |
| - Number must be positive and non-zero | |
| - Maximum allowed values: | |
| - Days: up to 365 | |
| - Hours: up to 8760 (equivalent to 365 days) | |
| - Invalid inputs will trigger error messages | |
| - Changes are persisted after validation |
…and-error-handling
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/AdvancedSettings/index.jsx (1)
241-262: Knees weak, arms heavy... this layout needs to be more steady!The JWT TTL input layout could be improved:
- Add spacing between number and units
- Consider full width for better alignment
- The
isHiddenprop on the units select seems unnecessary-<Stack direction="row" gap={theme.spacing(10)}> +<Stack + direction="row" + gap={theme.spacing(10)} + sx={{ width: '100%' }} +> <Field type="number" id="jwtTTLNum" label="JWT time to live" name="jwtTTLNum" value={localSettings.jwtTTLNum.toString()} onChange={handleChange} onBlur={handleBlur} error={errors.jwtTTLNum} + sx={{ flex: 2 }} /> <Select id="jwtTTLUnits" label="JWT TTL Units" name="jwtTTLUnits" placeholder="Select time" - isHidden={true} items={unitItems} value={unitItemLookup[localSettings.jwtTTLUnits]} onChange={handleJWTTTLUnits} onBlur={handleBlur} error={errors.jwtTTLUnits} + sx={{ flex: 1 }} /> </Stack>
| jwtTTLNum: 99, | ||
| jwtTTLUnits: "days", | ||
| jwtTTL: "99d", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, let's make this JWT TTL state initialization more robust!
The initial state values should be derived from the JWT TTL string parsing function to maintain consistency.
- jwtTTLNum: 99,
- jwtTTLUnits: "days",
- jwtTTL: "99d",
+ jwtTTLNum: "",
+ jwtTTLUnits: "",
+ jwtTTL: "",📝 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.
| jwtTTLNum: 99, | |
| jwtTTLUnits: "days", | |
| jwtTTL: "99d", | |
| jwtTTLNum: "", | |
| jwtTTLUnits: "", | |
| jwtTTL: "", |
| const parseJWTTTL = (data) => { | ||
| if (data.jwtTTL) { | ||
| const len = data.jwtTTL.length; | ||
| data.jwtTTLNum = data.jwtTTL.substring(0, len - 1); | ||
| data.jwtTTLUnits = unitItems.filter( | ||
| (itm) => itm._id == data.jwtTTL.substring(len - 1) | ||
| )[0].name; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Mom's spaghetti alert! Let's make this parsing more steady!
The JWT TTL parsing needs better error handling and validation:
- No validation for empty/null input
- No validation for numeric value
- No handling for invalid unit format
const parseJWTTTL = (data) => {
+ if (!data?.jwtTTL) return;
const len = data.jwtTTL.length;
+ const num = data.jwtTTL.substring(0, len - 1);
+ const unit = data.jwtTTL.substring(len - 1);
+
+ if (isNaN(num) || !unitItemLookup[unit]) {
+ return;
+ }
- data.jwtTTLNum = data.jwtTTL.substring(0, len - 1);
+ data.jwtTTLNum = num;
data.jwtTTLUnits = unitItems.filter(
- (itm) => itm._id == data.jwtTTL.substring(len - 1)
+ (itm) => itm._id === unitItemLookup[unit]
)[0].name;
};Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Looks good to me, just need those port vaildations to be numbers. Otherwise validation will be triggered when you try to save settings after loading initial settings:
EDIT
I also just realized after posting this screenshot that the label for system email port is wrong, it's showing system email address
|
Alright looking good! Thanks for making changes 😄 |
ajhollid
deleted the
913-fe-advanced-settings-page-validation-and-error-handling
branch
