Switch to libphonenumber-js;
#9969
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of phone number handling across the application by migrating from custom phone number validation to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
4d2cb17
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://781ce8f3.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-feat-switch-to.care-fe.pages.dev |
![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
🧹 Nitpick comments (3)
src/components/ui/input-phone.tsx (1)
16-21: Add proper accessibility attributes.The input lacks proper accessibility attributes for screen readers.
<Input type="tel" className={cn("pr-10", className)} ref={ref} + aria-label={props.placeholder ?? t("phone_number_placeholder")} {...props} maxLength={16}src/components/Common/SearchByMultipleFields.tsx (1)
183-193: Use internationalization for the error message.The error message "Invalid phone number" should be internationalized using the
tfunction for consistency with the rest of the application.Apply this diff to internationalize the error message:
- isValidPhoneNumber(value) ? undefined : "Invalid phone number", + isValidPhoneNumber(value) ? undefined : t("invalid_phone"),src/components/Facility/FacilityCreate.tsx (1)
88-89: Consider adding a custom error message.While the validation logic is correct, consider providing a more descriptive error message that explains the expected phone number format.
.refine(isValidPhoneNumber, t("invalid_phone_number")) +.refine(isValidPhoneNumber, t("phone_number_format_description"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
package.json(1 hunks)public/locale/en.json(1 hunks)src/Utils/utils.ts(0 hunks)src/common/constants.tsx(0 hunks)src/common/static/countryPhoneAndFlags.json(0 hunks)src/common/validation.tsx(0 hunks)src/components/Common/SearchByMultipleFields.tsx(3 hunks)src/components/Facility/CreateFacilityForm.tsx(5 hunks)src/components/Facility/FacilityCreate.tsx(5 hunks)src/components/Form/FieldValidators.tsx(0 hunks)src/components/Form/FormFields/FormField.tsx(1 hunks)src/components/Form/FormFields/PhoneNumberFormField.tsx(0 hunks)src/components/Form/FormFields/Utils.ts(1 hunks)src/components/Form/Utils.ts(1 hunks)src/components/Patient/PatientIndex.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(3 hunks)src/components/Resource/ResourceCreate.tsx(5 hunks)src/components/Users/UserListAndCard.tsx(3 hunks)src/components/ui/input-phone.tsx(1 hunks)src/pages/PublicAppointments/auth/PatientLogin.tsx(4 hunks)
💤 Files with no reviewable changes (6)
- src/common/validation.tsx
- src/common/static/countryPhoneAndFlags.json
- src/components/Form/FormFields/PhoneNumberFormField.tsx
- src/components/Form/FieldValidators.tsx
- src/Utils/utils.ts
- src/common/constants.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserListAndCard.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9864
File: src/components/Users/UserListAndCard.tsx:246-248
Timestamp: 2025-01-10T06:16:10.402Z
Learning: In the frontend codebase, the `formatPhoneNumber` utility function accepts undefined as input and returns "+91 " as the default output, making additional null checks unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (14)
src/components/Form/Utils.ts (1)
3-3: LGTM! Simplified error handling approach.The change from
FieldErrortostring | undefinedaligns well with the migration tolibphonenumber-js, providing a more straightforward error handling mechanism.Also applies to: 9-9
src/components/Form/FormFields/Utils.ts (1)
31-31: LGTM! Consistent error type update.The change to
string | undefinedmaintains consistency with the form utilities.src/components/Patient/PatientIndex.tsx (1)
Line range hint
1-103: Verify consistent phone number validation across the codebase.Let's ensure all phone number validations use libphonenumber-js consistently.
✅ Verification successful
Phone number validation is consistently implemented using libphonenumber-js
The codebase consistently uses libphonenumber-js for phone number validation, with appropriate functions chosen based on the use case:
parsePhoneNumberWithErrorfor parsing and strict validation in search/data processingisValidPhoneNumberfor form validation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential phone number validation patterns that might not be using libphonenumber-js rg -l "phone.*number" | xargs rg -l "validate|regex|pattern" | grep -v "node_modules" # Search for imports of parsePhoneNumber to ensure complete migration rg "import.*parsePhoneNumber(?!WithError)"Length of output: 912
Script:
#!/bin/bash # Search for libphonenumber-js imports rg "from ['\"]libphonenumber-js['\"]" # Check actual phone number validation logic in identified files for file in src/components/Auth/Login.tsx src/components/Facility/FacilityCreate.tsx src/components/Patient/PatientRegistration.tsx src/components/Users/CreateUserForm.tsx src/pages/PublicAppointments/PatientRegistration.tsx src/pages/PublicAppointments/auth/PatientLogin.tsx; do echo "=== $file ===" rg "phone.*number.*=|phone.*number.*\+" "$file" -A 2 -B 2 doneLength of output: 5101
src/pages/PublicAppointments/auth/PatientLogin.tsx (2)
77-77: LGTM! Clean and efficient phone number validation.The validation logic is concise and uses the reliable
libphonenumber-jslibrary with proper internationalization.
161-172: LGTM! Well-structured phone input implementation.The implementation follows best practices:
- Proper form field structure with label and error handling
- Clear error state on value change
- Consistent with the form component pattern
src/components/Facility/CreateFacilityForm.tsx (1)
263-263: LGTM! Clean phone input implementation.The implementation follows the form field pattern and integrates well with the form structure.
src/components/Resource/ResourceCreate.tsx (3)
3-6: LGTM! Clean import of required libphonenumber-js functions.The imports are correctly structured and only import the necessary functions.
407-410: LGTM! Clean migration to PhoneInput component.The component replacement is correct and maintains the form integration through onValueChange.
84-85: Verify error message translation key.The validation logic is correct, but ensure that the translation key 'invalid_phone_number' exists in the i18n files.
Run this script to check the translation key:
✅ Verification successful
Translation key 'invalid_phone_number' is properly configured
The key exists in the i18n files with translations in English, Tamil, Hindi, Kannada, and Malayalam.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the translation key exists in i18n files rg -g '*.json' '"invalid_phone_number":'Length of output: 395
src/components/Facility/FacilityCreate.tsx (2)
9-12: LGTM! Clean import of required libphonenumber-js functions.The imports are correctly structured and only import the necessary functions.
178-178: LGTM! Clean request data handling.The simplified request data handling is correct as the phone number transformation is now handled by the schema.
src/components/Form/FormFields/FormField.tsx (1)
32-32: LGTM! Simplified error type definition.The change to use
string | undefinedinstead ofFieldErrorsimplifies error handling and aligns with the form library's approach.package.json (1)
104-104: LGTM! Verify the package version.The addition of
libphonenumber-jsis appropriate for standardizing phone number handling. The library is well-maintained and widely used.Run the following script to check for any security advisories and verify the latest version:
✅ Verification successful
Package version verified - all checks passed
- Latest version (1.11.17) is being used
- No security vulnerabilities found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the libphonenumber-js library. # Check NPM for latest versions curl -s https://registry.npmjs.org/libphonenumber-js | jq -r '.["dist-tags"].latest' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "libphonenumber-js") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 445
public/locale/en.json (1)
1558-1558: LGTM! The phone number placeholder follows the correct format.The placeholder
+91xxxxxxxxxxclearly indicates the expected format for Indian phone numbers with the country code.
| value={props.value || "+91"} | ||
| placeholder={props.placeholder ?? t("phone_number_placeholder")} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the default country code configurable.
The hardcoded "+91" value limits the component's reusability for different regions. Consider:
- Making it a prop with "+91" as the default
- Using the user's locale to determine the default country code
- value={props.value || "+91"}
+ value={props.value || props.defaultCountry || "+91"}Committable suggestion skipped: line range outside the PR's diff.
| onChange={(e) => { | ||
| let value = e.target.value.replace(/[^\d+]/g, ""); | ||
| if (value && !value.startsWith("+")) { | ||
| value = "+" + value; | ||
| } | ||
| props.onValueChange?.(value); | ||
| props.onChange?.(e); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance phone number validation and formatting.
The current implementation only handles basic input sanitization. Consider:
- Using
parsePhoneNumberWithErrorfor real-time validation - Formatting the number as the user types
- Adding proper error handling for invalid numbers
onChange={(e) => {
- let value = e.target.value.replace(/[^\d+]/g, "");
- if (value && !value.startsWith("+")) {
- value = "+" + value;
- }
- props.onValueChange?.(value);
- props.onChange?.(e);
+ try {
+ let value = e.target.value.replace(/[^\d+]/g, "");
+ if (value && !value.startsWith("+")) {
+ value = "+" + value;
+ }
+ const phoneNumber = parsePhoneNumberWithError(value);
+ const formattedValue = phoneNumber.format("INTERNATIONAL");
+ props.onValueChange?.(formattedValue);
+ e.target.value = formattedValue;
+ props.onChange?.(e);
+ } catch (error) {
+ props.onValueChange?.(e.target.value);
+ props.onChange?.(e);
+ }
}}Committable suggestion skipped: line range outside the PR's diff.
| @@ -100,7 +100,7 @@ export default function PatientIndex({ facilityId }: { facilityId: string }) { | |||
| queryKey: ["patient-search", facilityId, phoneNumber], | |||
| queryFn: query.debounced(routes.searchPatient, { | |||
| body: { | |||
| phone_number: parsePhoneNumber(phoneNumber) || "", | |||
| phone_number: parsePhoneNumberWithError(phoneNumber).number.toString(), | |||
There was a problem hiding this comment.
Add error handling for invalid phone numbers.
Direct usage of parsePhoneNumberWithError without error handling could throw runtime exceptions for invalid numbers.
- phone_number: parsePhoneNumberWithError(phoneNumber).number.toString(),
+ phone_number: (() => {
+ try {
+ return parsePhoneNumberWithError(phoneNumber).number.toString();
+ } catch (error) {
+ return phoneNumber;
+ }
+ })(),📝 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.
| phone_number: parsePhoneNumberWithError(phoneNumber).number.toString(), | |
| phone_number: (() => { | |
| try { | |
| return parsePhoneNumberWithError(phoneNumber).number.toString(); | |
| } catch (error) { | |
| return phoneNumber; | |
| } | |
| })(), |
| @@ -215,7 +211,7 @@ const UserListRow = ({ user }: { user: UserBase }) => { | |||
| {user.user_type} | |||
| </td> | |||
| <td id="contact" className="px-4 py-4 text-sm whitespace-nowrap"> | |||
| {formatPhoneNumber(user.phone_number)} | |||
| {parsePhoneNumberWithError(user.phone_number).formatInternational()} | |||
There was a problem hiding this comment.
Add error handling for invalid phone numbers.
The current implementation may throw an error if the phone number is undefined or invalid. Consider adding error handling to gracefully handle such cases.
Apply this diff to add error handling:
- {parsePhoneNumberWithError(user.phone_number).formatInternational()}
+ {(() => {
+ try {
+ return parsePhoneNumberWithError(user.phone_number).formatInternational();
+ } catch (error) {
+ return user.phone_number || t("not_available");
+ }
+ })()}Committable suggestion skipped: line range outside the PR's diff.
| .refine(isValidPhoneNumber, "Invalid phone number") | ||
| .transform((value) => parsePhoneNumberWithError(value).number.toString()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance phone number validation and transformation.
The current implementation has two issues:
- The error message is not internationalized
- The transformation may throw an error for invalid phone numbers
Apply this diff to improve the implementation:
.string()
- .refine(isValidPhoneNumber, "Invalid phone number")
+ .refine(isValidPhoneNumber, t("invalid_phone"))
.transform((value) => {
+ try {
return parsePhoneNumberWithError(value).number.toString();
+ } catch (error) {
+ return value;
+ }
+ });Committable suggestion skipped: line range outside the PR's diff.
| @@ -759,7 +763,7 @@ export default function PatientRegistration( | |||
| </div> | |||
| {!patientPhoneSearch.isLoading && | |||
| !!duplicatePatients?.length && | |||
| !!parsePhoneNumber(debouncedNumber || "") && | |||
| !!parsePhoneNumberWithError(debouncedNumber || "").number.toString() && | |||
There was a problem hiding this comment.
Add error handling for duplicate check.
The duplicate check could throw an error for invalid numbers.
-!!parsePhoneNumberWithError(debouncedNumber || "").number.toString() &&
+(() => {
+ try {
+ return !!parsePhoneNumberWithError(debouncedNumber || "").number.toString();
+ } catch {
+ return false;
+ }
+})() &&📝 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.
| !!parsePhoneNumberWithError(debouncedNumber || "").number.toString() && | |
| (() => { | |
| try { | |
| return !!parsePhoneNumberWithError(debouncedNumber || "").number.toString(); | |
| } catch { | |
| return false; | |
| } | |
| })() && |
| phone_number: parsePhoneNumberWithError( | ||
| debouncedNumber || "", | ||
| ).number.toString(), |
There was a problem hiding this comment.
Add error handling for invalid phone numbers.
The phone number parsing could throw an error for invalid numbers. Consider adding try-catch.
-phone_number: parsePhoneNumberWithError(
- debouncedNumber || "",
-).number.toString(),
+phone_number: (() => {
+ try {
+ return parsePhoneNumberWithError(debouncedNumber || "").number.toString();
+ } catch {
+ return "";
+ }
+})(),📝 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.
| phone_number: parsePhoneNumberWithError( | |
| debouncedNumber || "", | |
| ).number.toString(), | |
| phone_number: (() => { | |
| try { | |
| return parsePhoneNumberWithError(debouncedNumber || "").number.toString(); | |
| } catch { | |
| return ""; | |
| } | |
| })(), |
| enabled: !!parsePhoneNumberWithError( | ||
| debouncedNumber || "", | ||
| ).number.toString(), |
There was a problem hiding this comment.
Add error handling for query enablement.
Similar to above, the enablement check could throw an error.
-enabled: !!parsePhoneNumberWithError(
- debouncedNumber || "",
-).number.toString(),
+enabled: (() => {
+ try {
+ return !!parsePhoneNumberWithError(debouncedNumber || "").number.toString();
+ } catch {
+ return false;
+ }
+})(),📝 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.
| enabled: !!parsePhoneNumberWithError( | |
| debouncedNumber || "", | |
| ).number.toString(), | |
| enabled: (() => { | |
| try { | |
| return !!parsePhoneNumberWithError(debouncedNumber || "").number.toString(); | |
| } catch { | |
| return false; | |
| } | |
| })(), |
Proposed Changes
libphonenumber-jsin favour of custom phone number validators #9966@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
libphonenumber-jslibrary for enhanced phone number handling and validationPhoneInputcomponent for improved phone number inputBug Fixes
Refactor
Documentation