Issue-2238: Fixes the error messages on various forms. #2239
Conversation
|
""" WalkthroughThe changes update error message handling and validation for user authentication forms. Error messages for email fields in the Forgot Password and Register forms are now consistently passed through the translation function. A dedicated surname validation schema is introduced, and validation schemas are reformatted for clarity without altering logic. Changes
Assessment against linked issues
Why did the Canadian developer apologize after fixing the translation bug? Because even their code says "sorry" when it gets things right, eh! Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
client/src/Validation/validation.js (1)
175-175: Ignore static analysis warning about 'then' propertyThe static analysis warning about "Do not add then to an object" appears to be a false positive. This is standard Joi validation syntax where
.when()takes an object withisandthenproperties. No change needed.You could add a comment to suppress this specific linting warning if it shows up in your CI pipeline:
.when("type", { + // biome-ignore lint/suspicious/noThenProperty: This is valid Joi syntax is: "port", then: joi.required().messages({🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/Pages/Auth/ForgotPassword.jsx(1 hunks)client/src/Pages/Auth/Register/StepTwo/index.jsx(1 hunks)client/src/Validation/validation.js(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/Validation/validation.js
[error] 175-175: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (8)
client/src/Pages/Auth/ForgotPassword.jsx (1)
188-188: Improved localization of error messages - good job!Nice work passing the error message through the translation function. This ensures consistent localization of validation messages across the application, improving the experience for international users.
Just like an American and Canadian agreeing on hockey (but not how to spell "color"), this change brings harmony to your error handling!
client/src/Pages/Auth/Register/StepTwo/index.jsx (1)
71-78: Well-structured error message translation logic!This refactoring improves the error message handling by:
- Ensuring all error messages go through the translation function
- Using specific translation keys for common errors
- Applying a hierarchical approach to message selection
This is a more robust approach that aligns with the same pattern applied in the ForgotPassword component.
As a Canadian might say, this code is now as organized as a Tim Hortons drive-thru line during morning rush!
client/src/Validation/validation.js (6)
18-28: Great introduction of a dedicated surname schema!Creating a dedicated schema for surnames with specific error messages improves the user experience by providing clearer validation feedback. Users will now see "Surname is required" instead of the generic "Name is required" when the last name field is empty.
This reminds me of how Americans and Canadians both speak English, but with just enough differences to notice - like your schemas now!
69-69: Proper integration of surname schema - good!This change correctly implements the new
surnameSchemafor thelastNamefield, ensuring users receive surname-specific validation messages.
269-278: Improved formatting for settings validation schemaThe reformatting of the
settingsValidationschema with consistent indentation and structure improves code readability and maintainability.
381-385: Enhanced readability for email validationThe multiline formatting of the email address validation improves code readability while maintaining the same validation rules.
398-398: Added trailing comma in export listAdding a trailing comma in the export list is a good practice that makes future additions cleaner in git diffs. Like a Canadian apologizing for something that wasn't their fault, this little comma makes everything more polite!
168-182:❓ Verification inconclusive
Improved port validation schema formatting
The reformatting of the port validation schema improves code readability without changing functionality. The explicit
otherwise: joi.optional()clause adds clarity to the conditional validation logic.You know what they say - Americans and Canadians may disagree on whether it's called a "washroom" or "restroom," but they both agree that readable code is important!
🏁 Script executed:
#!/bin/bash # Description: Verify if there are any other Joi validation schemas that could benefit from similar explicit otherwise clauses # Look for .when() calls that might not have explicit otherwise clauses rg -n "\.when\([^)]+\)\s*\.\s*[a-zA-Z]+" --type js client/src/ValidationLength of output: 74
Conditional Joi validations check
I ran a quick search for other
.when()calls inclient/src/Validationand didn’t find any without an explicitotherwiseclause. It looks like this is the only schema that needs the explicitotherwise: joi.optional()for clarity, but please give it a quick once-over to be sure there aren’t any edge cases missed.Canadians may call it a “washroom,” Americans call it a “restroom,” but both agree that validating ports from 1–65535 is non-negotiable!
🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Do not add then to an object.
(lint/suspicious/noThenProperty)
![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: Fix translation issues and validation messages across authentication forms
- Key components: ForgotPassword, Register StepTwo, validation schemas
- Cross-component impacts: Internationalization system integration, validation logic consistency
- Business value: Improved user experience through clear localized error messages
1.2 Technical Architecture
- System design: Enhanced validation message handling in UI layer
- Integration points: Joi validation schemas now drive localized error messages
- Dependency changes: Modified schema definitions impact form validation across multiple flows
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Incorrect error message fallback in Register StepTwo email validation
- Analysis Confidence: High
- Impact: Untranslated raw error strings displayed to users
- Resolution: Revert fallback to original error message display
Issue: Empty error messages in infrastructureMonitorValidation
- Analysis Confidence: High
- Impact: Missing validation feedback for infrastructure monitors
- Resolution: Remove empty .messages() call in validation schema
2.2 Should Fix (P1🟡)
Issue: Brittle error message matching via string.includes()
- Analysis Confidence: Medium
- Impact: Prone to breakage if validation messages change
- Suggested Solution: Implement error code mapping in validation schemas
2.3 Consider (P2🟢)
Area: Test coverage for validation messages
- Analysis Confidence: High
- Improvement Opportunity: Ensure message stability across locales
2.4 Summary of Action Items
- Fix Register StepTwo fallback (P0) - Immediate
- Restore infrastructure monitor messages (P0) - Immediate
- Refactor error matching logic (P1) - Next sprint
- Add validation message tests (P2) - Future
3. Technical Analysis
3.1 Code Logic Analysis
📁 client/src/Pages/Auth/Register/StepTwo/index.jsx - Email validation
- Submitted PR Code:
helperText={
errors.email &&
(errors.email.includes("required")
? t("authRegisterEmailRequired")
: errors.email.includes("valid email")
? t("authRegisterEmailInvalid")
: t(errors.email))
}- Analysis:
- Fallback translation attempt of raw Joi messages
- Breaks non-specifically handled error display
- Creates dependency on validation message content
- LlamaPReview Suggested Improvements:
helperText={
errors.email &&
(errors.email.includes("required")
? t("authRegisterEmailRequired")
: errors.email.includes("valid email")
? t("authRegisterEmailInvalid")
: errors.email) // Reverted fallback
}- Improvement rationale:
- Preserves original validation messages
- Avoids incorrect translation attempts
📁 client/src/Validation/validation.js - Infrastructure monitor validation
- Submitted PR Code:
notifications: joi.array().items(
joi.object({
type: joi.string().valid("email").required(),
address: joi.string().email({ tlds: { allow: false } }).required(),
})
),- Analysis:
- Missing error message customization
- Removed default Joi validation messages
- Users receive empty error feedback
- LlamaPReview Suggested Improvements:
notifications: joi.array().items(
joi.object({
type: joi.string().valid("email").required()
.messages({ 'any.only': 'Invalid notification type' }),
address: joi.string().email({ tlds: { allow: false } }).required()
.messages({ 'string.email': 'Invalid email format' }),
})
).messages({
'array.base': 'Notifications must be an array'
}),- Improvement rationale:
- Restores critical validation feedback
- Maintains consistent error messaging
4. Overall Evaluation
- Technical assessment: Addresses core issues with some implementation gaps
- Business impact: High value UX improvement when fixes applied
- Risk evaluation: Medium risk without test coverage
- Positive aspects: Good schema separation, improved i18n integration
- Implementation quality: Requires critical fixes before merging
- Final Recommendation: Request Changes (P0 fixes required)
💡 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.
client/src/Validation/validation.js
Outdated
| pagespeedApiKey: joi.string().allow("").optional(), | ||
| }) | ||
| .unknown(true); | ||
| const settingsValidation = joi |
There was a problem hiding this comment.
@sonvir249 , why is this getting formatted, please try and format those codes which you have changed or else the PR looks long for no reason.
There was a problem hiding this comment.
@Owaiseimdad Removed this formatting change.
| @@ -185,7 +185,7 @@ const ForgotPassword = () => { | |||
| value={form.email} | |||
| onChange={handleChange} | |||
| error={errors.email ? true : false} | |||
| helperText={errors.email} | |||
| helperText={t(errors.email)} | |||
There was a problem hiding this comment.
Any particular reason for this change? Also, can you share a UI SS.
There was a problem hiding this comment.
@Br0wnHammer I added the detailed screenshots in the issue #2238. I've attached the SS in PR too. Here errors.email returns the authRegisterEmailInvalid and authRegisterEmailRequired and it should be passed in t() to print the translation string.
| ? t("authRegisterEmailRequired") | ||
| : errors.email.includes("valid email") | ||
| ? t("authRegisterEmailInvalid") | ||
| : t(errors.email)) |
There was a problem hiding this comment.
@Br0wnHammer removed unnecessary changes.
client/src/Validation/validation.js
Outdated
| @@ -53,7 +66,7 @@ const passwordSchema = joi | |||
|
|
|||
| const credentials = joi.object({ | |||
| firstName: nameSchema, | |||
| lastName: nameSchema, | |||
| lastName: surnameSchema, | |||
There was a problem hiding this comment.
A better name would be lastnameSchema.
client/src/Validation/validation.js
Outdated
| port: joi | ||
| .number() | ||
| .integer() | ||
| .min(1) |
There was a problem hiding this comment.
@Br0wnHammer removed unnecessary change
client/src/Validation/validation.js
Outdated
| }), | ||
| pagespeedApiKey: joi.string().allow("").optional(), | ||
| }) | ||
| .unknown(true); |
There was a problem hiding this comment.
Again, please refrain from using formatting on the code that is not supported by us. Makes the diff larger than it is.
There was a problem hiding this comment.
@Br0wnHammer removed unnecessary change
client/src/Validation/validation.js
Outdated
| @@ -377,5 +395,5 @@ export { | |||
| advancedSettingsValidation, | |||
| infrastructureMonitorValidation, | |||
| statusPageValidation, | |||
| logoImageValidation | |||
| logoImageValidation, | |||
There was a problem hiding this comment.
@Br0wnHammer removed unnecessary change
client/src/Validation/validation.js
Outdated
| "Name must contain only letters, spaces, apostrophes, or hyphens", | ||
| }); | ||
|
|
||
| const surnameSchema = joi |
There was a problem hiding this comment.
@sonvir249 , it looks like the same Joi schema is being repeated. Let's simplify this by renaming it to nameStringSchema. Also, instead of using name and surname, let's generalize it to something like characters or simply indicate that input is required. Please make sure to apply the DRY (Don't Repeat Yourself) principle wherever applicable.
There was a problem hiding this comment.
@Br0wnHammer removed unnecessary change
| ? t("authRegisterEmailRequired") | ||
| : errors.email.includes("valid email") | ||
| ? t("authRegisterEmailInvalid") | ||
| : t(errors.email)) |
There was a problem hiding this comment.
@sonvir249 does anything like t(errors.email) exists in local file and, is it even intented to be from translation or is it just errors.email?
There was a problem hiding this comment.
@Owaiseimdad errors.email returns the authRegisterEmailInvalid and authRegisterEmailRequired and it should be passed in t() to print the translation string.
| @@ -185,7 +185,7 @@ const ForgotPassword = () => { | |||
| value={form.email} | |||
| onChange={handleChange} | |||
| error={errors.email ? true : false} | |||
| helperText={errors.email} | |||
| helperText={t(errors.email)} | |||
There was a problem hiding this comment.
same here, I don't see anything like errors.email in translation file, let me know if I am missing anything here.
@sonvir249
There was a problem hiding this comment.
@Owaiseimdad errors.email returns the authRegisterEmailInvalid and authRegisterEmailRequired and it should be passed in t() to print the translation string.
There was a problem hiding this comment.
@sonvir249 please make your changes and test it locally and add screenshots for reference.
sonvir249
force-pushed
the
bug/fix-form-errors
branch
from
4efbd4b to
f0d2c57
Compare
@Owaiseimdad @Owaiseimdad thanks for the review and detailed comments. I appreciate that. |
Thanks @sonvir249. The screenshot is for the PR, not the issue, eg. reflects what you did rather than what the issue is. For this reason we are asking it to be added to the PR instead. |
|
@ajhollid this is fine for merging. |
Describe your changes
Fixes the error message on signup and reset form.
Write your issue number after "Fixes "
Fixes #2238
Screenshots
Summary by CodeRabbit
New Features
Refactor