-
Notifications
You must be signed in to change notification settings - Fork 649
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
Add max phone number length to user creation form #9952
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe pull request implements input validation enhancements for phone number fields across three components: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
Deploying care-fe with
|
Latest commit: |
0c9114a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6f5331ce.care-fe.pages.dev |
Branch Preview URL: | https://maxlength-userform.care-fe.pages.dev |
LGTM |
CARE
|
Project |
CARE
|
Branch Review |
maxlength-userform
|
Run status |
|
Run duration | 01m 47s |
Commit |
|
Committer | Mohammed Nihal |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
4
|
View all changes introduced in this branch ↗︎ |
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.
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: 1
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
315-320
: Consider input masking for better user experience.While the maxLength restriction works, consider using an input mask library to:
- Format the input as user types
- Prevent invalid characters
- Guide users with visual feedback
Consider using a library like
react-input-mask
orreact-number-format
:<InputMask mask="+91 9999999999" type="tel" placeholder="+91XXXXXXXXXX" {...field} />
337-337
: Redundant maxLength validation.The maxLength attribute is redundant since the form's validation schema already enforces the exact format with regex
/^\+91[0-9]{10}$/
. Consider removing maxLength or updating the validation strategy to be consistent.Either:
- Remove maxLength and rely on regex validation
- Or replace regex with simpler validation and keep maxLength:
phone_number: z .string() .min(13, t("invalid_phone_length")) .max(13, t("invalid_phone_length")) .startsWith("+91", t("must_start_with_country_code"))
<Input | ||
type="tel" | ||
placeholder="+91XXXXXXXXXX" | ||
maxLength={13} | ||
{...field} | ||
/> |
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
Phone number validation requirements are overly restrictive
The current implementation enforces a strict Indian phone number format, but the codebase has utilities (parsePhoneNumber
, formatPhoneNumber
) that support more flexible phone number handling. Consider:
- Using the existing phone utilities to allow formatted input (spaces, hyphens)
- Supporting international numbers since the codebase already has this capability
- Maintaining +91 validation only if it's a strict business requirement
Example locations that handle phone numbers more flexibly:
src/Utils/utils.ts
:parsePhoneNumber()
andformatPhoneNumber()
src/pages/PublicAppointments/auth/PatientLogin.tsx
: Phone validation with formatting
🔗 Analysis chain
Verify phone number validation requirements.
The current implementation enforces:
- Exactly 13 characters
- Must start with +91
- Exactly 10 digits after +91
Please confirm if these strict requirements align with the business needs:
- Should international numbers be supported?
- Should spaces or hyphens be allowed for formatting?
- Should shorter emergency numbers be allowed?
Also applies to: 337-337
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other phone number validation patterns in the codebase
rg -l "phone.*validation|phone.*regex" --type ts --type tsx
Length of output: 89
Script:
#!/bin/bash
# Search for phone-related validation patterns in TypeScript files
rg -t typescript -t javascriptreact -t typescriptreact "phone.*validation|phone.*regex|phoneNumber|phone.*schema" -A 5
# Search for form validation schemas
rg -t typescript -t typescriptreact "schema.*phone|validate.*phone" -A 5
# Search for test files related to phone validation
fd ".*\.(test|spec)\.(ts|tsx)$" --exec rg -l "phone|mobile"
Length of output: 327
Script:
#!/bin/bash
# Search for validation schema in CreateUserForm.tsx
rg "phone" src/components/Users/CreateUserForm.tsx -A 5 -B 5
# Search for validation-related imports and schemas
rg "import.*yup|import.*zod|schema.*=|validation.*=|phone.*=" src/components/Users/CreateUserForm.tsx
# Search for phone-related patterns in the entire codebase
rg "phone.*validation|phoneNumber|phone.*schema" --type-add 'ts:*.{ts,tsx}' -t ts
Length of output: 12071
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
@nihal467 there are places where |
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: 1
🧹 Nitpick comments (2)
src/components/Facility/CreateFacilityForm.tsx (1)
266-266
: LGTM! Consider enhancing validation feedback.The changes improve user experience by:
- Restricting input length to match the required format (+91 + 10 digits)
- Optimizing mobile input with
type="tel"
Consider adding pattern validation to provide immediate feedback before form submission:
<Input type="tel" data-cy="facility-phone" placeholder="+91XXXXXXXXXX" maxLength={13} + pattern="^\+91[0-9]{10}$" {...field} />
src/components/Patient/PatientRegistration.tsx (1)
395-395
: LGTM! Consider enhancing validation feedback.The changes improve user experience by:
- Restricting input length appropriately
- Optimizing mobile input with
type="tel"
Consider adding pattern validation to provide immediate feedback before form submission:
<Input type="tel" {...field} maxLength={13} + pattern="^\+\d{12}$" data-cy="patient-emergency-phone-input" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/CreateFacilityForm.tsx
(1 hunks)src/components/Patient/PatientRegistration.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
@@ -333,6 +333,7 @@ export default function PatientRegistration( | |||
<FormLabel required>{t("phone_number")}</FormLabel> | |||
<FormControl> | |||
<Input | |||
type="tel" |
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
Phone number validation should be aligned with India-specific format
The current validation pattern /^\+\d{12}$/
allows any country code, but the application is designed specifically for Indian phone numbers:
- All other components enforce the pattern
/^\+91[0-9]{10}$/
- UI messages explicitly state "Phone number must start with +91 followed by 10 digits"
- Common validation utilities are India-specific
Consider updating the validation to maintain consistency:
.regex(/^\+91[0-9]{10}$/, t("phone_number_validation"))
🔗 Analysis chain
LGTM! Consider standardizing phone validation across components.
The changes improve UX, but there's an inconsistency in phone number validation:
- This component allows any country code:
/^\+\d{12}$/
- CreateFacilityForm requires +91:
/^\+91[0-9]{10}$/
Let's check for other phone number validation patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for phone number validation patterns
rg -A 2 'phone.*regex'
Length of output: 24
Script:
#!/bin/bash
# Search for phone validation patterns with broader terms
echo "=== Phone validation patterns ==="
rg -i "phone.*valid|valid.*phone" -A 2
echo -e "\n=== Zod phone schemas ==="
rg "phone.*z\." -A 2
echo -e "\n=== General phone patterns ==="
rg -i "phone.*pattern|phone.*match" -A 2
Length of output: 8460
Script:
#!/bin/bash
# Check phonePreg implementation
echo "=== phonePreg implementation ==="
rg -A 2 "phonePreg.*="
echo -e "\n=== Phone regex constants ==="
rg -A 2 "PHONE.*REGEX.*="
Length of output: 1148
@nihal467 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit