Switch to libphonenumber-js in favour of custom phone number validators #9989
Conversation
…feat/switched-to-libphonenumber-js
…feat/switched-to-libphonenumber-js
WalkthroughThis pull request introduces a comprehensive migration from custom phone number validation to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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. |
|
@rithviknishad I found a bug in this branch, the search patient page is not working and behaves as follow, not sure what causing this Screen.Recording.2025-01-15.030753.mp4 |
|
oh that was what i commented in the issue; error handling. hint is there in the console logs 😅 |
…feat/switched-to-libphonenumber-js
…feat/switched-to-libphonenumber-js
|
@rithviknishad I think all files are done, is there any changes required ?🤔 |
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…feat/switched-to-libphonenumber-js
| phone_number: parsePhoneNumber(data.phone_number), | ||
| geo_organization: organizationId, | ||
| }); | ||
| createFacility({ ...data, geo_organization: organizationId }); |
There was a problem hiding this comment.
@rithviknishad in recent commit of develop branch the phone_number is used, should I keep geo_organization or phone_number?
github-actions
bot
added
merge conflict
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…feat/switched-to-libphonenumber-js
|
@manmeetnagii what is the status of this PR |
@nihal467 Requested for review |
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (4)
package.json(1 hunks)public/locale/en.json(1 hunks)src/Utils/utils.ts(0 hunks)src/components/Facility/FacilityForm.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/Utils/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- public/locale/en.json
🧰 Additional context used
🪛 GitHub Check: lint
src/components/Facility/FacilityForm.tsx
[failure] 28-28:
Delete ⏎
🪛 GitHub Check: cypress-run (1)
src/components/Facility/FacilityForm.tsx
[failure] 28-28:
Delete ⏎
🪛 GitHub Actions: Lint Code Base
src/components/Facility/FacilityForm.tsx
[error] 28-28: Prettier formatting error: Unexpected line break detected
🪛 GitHub Actions: Cypress Tests
src/components/Facility/FacilityForm.tsx
[error] 28-28: Delete newline character - Prettier formatting error
🔇 Additional comments (4)
src/components/Facility/FacilityForm.tsx (4)
3-6: LGTM! Proper integration of libphonenumber-js.The imports are correctly structured, importing only the necessary functions from libphonenumber-js.
Also applies to: 26-26
80-81: Use translation function for error message.The error message "Invalid phone number" should use the translation function for consistency.
- .refine(isValidPhoneNumber, "Invalid phone number") + .refine(isValidPhoneNumber, t("invalid_phone_number"))
331-331: LGTM! Proper integration of PhoneInput component.The PhoneInput component is correctly integrated with proper form controls and testing attributes.
145-147: Add error handlers to mutation hooks.The mutation hooks lack error handling for API failures, which could lead to a poor user experience.
- updateFacility({ ...data, geo_organization: props.organizationId }); + updateFacility( + { ...data, geo_organization: props.organizationId }, + { + onError: (error) => { + toast.error(t("failed_to_update_facility")); + console.error(error); + }, + } + ); - createFacility({ ...data, geo_organization: props.organizationId }); + createFacility( + { ...data, geo_organization: props.organizationId }, + { + onError: (error) => { + toast.error(t("failed_to_create_facility")); + console.error(error); + }, + } + );
| import { MultiSelect } from "@/components/ui/multi-select"; | ||
|
|
There was a problem hiding this comment.
Remove empty line to fix pipeline failures.
The empty line is causing pipeline failures in both the linter and Cypress tests.
import { MultiSelect } from "@/components/ui/multi-select";
-
import {📝 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.
| import { MultiSelect } from "@/components/ui/multi-select"; | |
| import { |
🧰 Tools
🪛 GitHub Check: lint
[failure] 28-28:
Delete ⏎
🪛 GitHub Check: cypress-run (1)
[failure] 28-28:
Delete ⏎
🪛 eslint
[error] 28-29: Delete ⏎
(prettier/prettier)
🪛 GitHub Actions: Lint Code Base
[error] 28-28: Prettier formatting error: Unexpected line break detected
🪛 GitHub Actions: Cypress Tests
[error] 28-28: Delete newline character - Prettier formatting error
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
only working PRs will/can be reviewed :) |
manmeetnagii
force-pushed
the
feat/switched-to-libphonenumber-js
branch
from
3eff88e to
83921f3
Compare
…feat/switched-to-libphonenumber-js
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Users/UserForm.tsx (1)
Line range hint
367-382: Remove maxLength restriction from phone number inputs.The
maxLength={13}restriction might prevent valid phone numbers from being entered, as phone numbers can vary in length across different countries. For example, phone numbers in some countries can be longer than 13 characters when including the country code.<Input data-cy="phone-number-input" type="tel" placeholder="+91XXXXXXXXXX" - maxLength={13} {...field} /><Input data-cy="alt-phone-number-input" placeholder="+91XXXXXXXXXX" type="tel" - maxLength={13} {...field} disabled={isWhatsApp} />
♻️ Duplicate comments (2)
src/components/Patient/PatientIndex.tsx (1)
100-114:⚠️ Potential issueEnhance error handling and request lifecycle management.
The current implementation has several issues that need to be addressed:
- The AbortController is not properly managed
- Error handling is incomplete
- Error messages are not internationalized
- The function might return undefined on error
Apply this diff to fix these issues:
queryFn: () => { + const controller = new AbortController(); try { const response = query.debounced(routes.searchPatient, { body: { phone_number: parsePhoneNumberWithError(phoneNumber).number.toString(), }, - })({ signal: new AbortController().signal }); + })({ signal: controller.signal }); return response; } catch (error) { if (error instanceof ParseError) { - toast.error("Invalid Phone Number"); + toast.error(t("invalid_phone_number")); + } else if (error instanceof Error && error.name !== 'AbortError') { + toast.error(t("error_searching_patient")); } + throw error; } + return () => controller.abort(); },Key improvements:
- Proper AbortController lifecycle management
- Comprehensive error handling including network errors
- Internationalized error messages using the t() function
- Proper error propagation to React Query
src/components/Resource/ResourceCreate.tsx (1)
84-85:⚠️ Potential issueAdd error handling for phone number parsing
The current implementation might throw an uncaught exception if
parsePhoneNumberWithErrorfails. Consider wrapping it in a try-catch block..refine(isValidPhoneNumber, t("invalid_phone_number")) -.transform((value) => parsePhoneNumberWithError(value).number.toString()), +.transform((value) => { + try { + return parsePhoneNumberWithError(value).number.toString(); + } catch (error) { + throw new Error(t("invalid_phone_number")); + } +}),
🧹 Nitpick comments (1)
src/components/Users/UserForm.tsx (1)
90-93: Duplicate validation logic detected.The validation logic for alt_phone_number is identical to phone_number. Consider extracting this into a reusable schema.
+const phoneNumberSchema = z + .string() + .refine(isValidNumber, t("phone_number_validation")) + .transform((value) => { + try { + return parsePhoneNumberWithError(value).number.toString(); + } catch (error) { + return value; + } + }); const userFormSchema = z .object({ - phone_number: z - .string() - .refine(isValidNumber, t("phone_number_validation")) - .transform((value) => - parsePhoneNumberWithError(value).number.toString(), - ), - alt_phone_number: z - .string() - .refine(isValidNumber, t("phone_number_validation")) - .transform((value) => - parsePhoneNumberWithError(value).number.toString(), - ) - .optional(), + phone_number: phoneNumberSchema, + alt_phone_number: phoneNumberSchema.optional(), })
📜 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 (8)
package.json(1 hunks)public/locale/en.json(1 hunks)src/common/constants.tsx(0 hunks)src/components/Facility/FacilityForm.tsx(5 hunks)src/components/Patient/PatientIndex.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(4 hunks)src/components/Resource/ResourceCreate.tsx(5 hunks)src/components/Users/UserForm.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- src/common/constants.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/components/Facility/FacilityForm.tsx
- public/locale/en.json
🔇 Additional comments (10)
src/components/Patient/PatientRegistration.tsx (4)
3-7: LGTM! Good choice of library.The migration to
libphonenumber-jsis a solid improvement for phone number validation and parsing.
266-281: LGTM! Good error handling.The phone number search query implementation properly handles parsing errors and shows appropriate error messages.
847-849: Add safety check for phone number parsing.This is a duplicate of a previous review comment. Direct usage of
parsePhoneNumberWithErrorin JSX could cause runtime errors. Consider adding a safety check as suggested in the previous review.
Line range hint
1-858: Consider adding E2E tests for phone number validation.The changes introduce new phone number validation logic. It would be beneficial to add end-to-end tests to verify:
- Valid phone number formats are accepted
- Invalid formats show appropriate error messages
- Emergency contact number validation works correctly
- Duplicate patient detection works with various phone formats
Would you like me to help create these test cases?
src/components/Patient/PatientIndex.tsx (1)
2-2: LGTM: Import of libphonenumber-js.The import aligns with the PR objective of switching to libphonenumber-js for phone number validation.
src/components/Resource/ResourceCreate.tsx (3)
3-6: LGTM! Clean import of libphonenumber-js functions.The imports are correctly structured and align with the migration to libphonenumber-js.
129-129: LGTM! Correct usage of transformed phone number.The phone number is properly handled in the submit handler, utilizing the validated and transformed value from the form data.
407-409: Verify PhoneInput component implementationThe migration to PhoneInput looks good, but let's verify its implementation.
✅ Verification successful
PhoneInput implementation and usage verified
The component correctly implements the
onValueChangeprop and the usage in ResourceCreate.tsx follows the established pattern across the codebase. The implementation includes proper phone number formatting and form integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify PhoneInput component implementation and its usage across the codebase # Check if PhoneInput component exists and examine its implementation echo "Checking PhoneInput component implementation..." rg -A 10 "export.*PhoneInput" "src/components/ui/" # Find all usages of PhoneInput to ensure consistent usage echo "Checking PhoneInput usage patterns..." rg "PhoneInput" "src/"Length of output: 1280
Script:
#!/bin/bash # Examine PhoneInput implementation echo "PhoneInput Implementation:" cat src/components/ui/input-phone.tsx # Check other usage patterns for comparison echo -e "\nOther Usage Patterns:" rg -B 2 -A 2 "<PhoneInput" src/Length of output: 2887
src/components/Users/UserForm.tsx (2)
3-3: LGTM! Clean import of libphonenumber-js functions.The necessary functions are correctly imported from libphonenumber-js.
84-93: Verify consistent phone number validation across the codebase.Let's ensure that all phone number validations in the codebase have been migrated to use libphonenumber-js.
✅ Verification successful
Phone number validation is consistently implemented using libphonenumber-js
The codebase already uses libphonenumber-js for phone number validation across multiple components including user registration, patient registration, facility forms, and resource creation. The implementation in UserForm.tsx follows the same pattern used throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other phone number validations that might need updating echo "Searching for phone number related validations..." rg -l "phone.*validation|validatePhone|isValidPhone" --type ts --type tsx echo "Searching for phone number related tests..." rg -l "phone.*test|test.*phone" --type test echo "Searching for other instances of maxLength={13} restrictions on phone inputs..." ast-grep --pattern 'maxLength={13}'Length of output: 591
Script:
#!/bin/bash # Search for phone-related code and validations echo "Searching for phone number related code..." rg -l "phone|validatePhone|isValidPhone" -g '*.{ts,tsx}' echo "Searching for libphonenumber-js usage..." rg -l "libphonenumber-js" -g '*.{ts,tsx}' echo "Searching for phone-related test files..." rg -l "phone.*test|test.*phone" -g '*.test.{ts,tsx}' echo "Searching for phone input components..." ast-grep --pattern 'input $$$phone$$$'Length of output: 3302
| .refine(isValidPhoneNumber, t("phone_number_must_be_10_digits")) | ||
| .transform((value) => | ||
| parsePhoneNumberWithError(value).number.toString(), | ||
| ), |
There was a problem hiding this comment.
Add error handling for phone number parsing.
Direct usage of parsePhoneNumberWithError in the transform function could throw runtime errors and crash the form validation.
Apply this diff to handle parsing errors safely:
phone_number: z
.string()
.refine(isValidPhoneNumber, t("phone_number_must_be_10_digits"))
.transform((value) =>
- parsePhoneNumberWithError(value).number.toString(),
+ {
+ try {
+ return parsePhoneNumberWithError(value).number.toString();
+ } catch (error) {
+ return value;
+ }
+ }
),Apply the same change to emergency_phone_number field as well.
Also applies to: 102-105
| .refine(isValidNumber, t("phone_number_validation")) | ||
| .transform((value) => | ||
| parsePhoneNumberWithError(value).number.toString(), | ||
| ), |
There was a problem hiding this comment.
Add error handling for phone number parsing.
While the validation is more robust using libphonenumber-js, the parsePhoneNumberWithError can throw if the number is invalid. Consider wrapping the transform in a try-catch block.
.refine(isValidNumber, t("phone_number_validation"))
.transform((value) =>
- parsePhoneNumberWithError(value).number.toString(),
+ {
+ try {
+ return parsePhoneNumberWithError(value).number.toString();
+ } catch (error) {
+ 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.
| .refine(isValidNumber, t("phone_number_validation")) | |
| .transform((value) => | |
| parsePhoneNumberWithError(value).number.toString(), | |
| ), | |
| .refine(isValidNumber, t("phone_number_validation")) | |
| .transform((value) => | |
| { | |
| try { | |
| return parsePhoneNumberWithError(value).number.toString(); | |
| } catch (error) { | |
| return value; | |
| } | |
| }, | |
| ), |
github-actions
bot
added
merge conflict
|
👋 Hi, @manmeetnagii, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…feat/switched-to-libphonenumber-js
|
@rithviknishad @Jacobjeevan review please |
It is showing cypress tests and auto label conflicts check fails |
users section |
…feat/switched-to-libphonenumber-js
|
@AdityaJ2305 changes done |
rithviknishad
changed the base branch from
develop
to
rithviknishad/feat/switch-to-libphonenumberjs
rithviknishad
changed the base branch from
rithviknishad/feat/switch-to-libphonenumberjs
to
develop
|
Closing in favour of #10214 |
Proposed Changes
libphonenumber-jsin favour of custom phone number validators #9966@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
libphonenumber-jslibraryPhoneInputcomponent for improved phone number input experienceImprovements
Changes
Deprecations