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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/components/Patient/PatientIndex.tsx (1)
100-114: 🛠️ Refactor suggestionEnhance error handling and provide comprehensive feedback
The current error handling in the
queryFnonly catchesParseErrorand does not handle other potential errors such as network issues or aborted requests. Additionally, theAbortControlleris instantiated but not properly utilized for canceling requests.Consider the following improvements:
- Use the
AbortControllereffectively: Store the controller outside thequeryFnto allow cancellation of the request when needed.- Handle additional errors: Provide user feedback for other error types, such as network errors or abort errors.
- Return a cleanup function: If using
AbortController, return a cleanup function to abort the request when the component unmounts.Apply this diff to implement the suggested changes:
const { data: patientList, isFetching } = useQuery({ queryKey: ["patient-search", facilityId, phoneNumber], queryFn: () => { - try { - const response = query.debounced(routes.searchPatient, { - body: { - phone_number: - parsePhoneNumberWithError(phoneNumber).number.toString(), - }, - })({ signal: new AbortController().signal }); - return response; - } catch (error) { - if (error instanceof ParseError) { - toast.error("Invalid Phone Number"); - } - } + const controller = new AbortController(); + const signal = controller.signal; + return query + .debounced(routes.searchPatient, { + body: { + phone_number: + parsePhoneNumberWithError(phoneNumber).number.toString(), + }, + })({ signal }) + .catch((error) => { + if (error instanceof ParseError) { + toast.error(t("invalid_phone_format", { example: "+91 XXXXX XXXXX" })); + } else if (error.name !== "AbortError") { + toast.error(t("error_fetching_patient_data")); + } + throw error; + }); + }, + onError: (error) => { + console.error("Error fetching patient data:", error); + }, + onSettled: () => { + // Any cleanup or additional logic can be placed here + }, + enabled: !!phoneNumber, + refetchOnWindowFocus: false, });src/components/Users/CreateUserForm.tsx (1)
81-84:⚠️ Potential issueAdd consistent error handling for alternate phone number.
The same error handling issue exists in the alternate phone number validation.
Apply the same error handling pattern as suggested for the primary phone number.
🧹 Nitpick comments (4)
vite.config.mts.timestamp-1737379816854-14baa337b2fe5.mjs (3)
169-172: Fix formatting issues in the array of CDN URLsThe array of CDN URLs
cdnUrlsis not properly formatted according to Prettier guidelines. Align the array elements properly to improve code readability.Apply this diff to fix the formatting:
const cdnUrls = - env.REACT_CDN_URLS || [ - "https://egov-s3-facility-10bedicu.s3.amazonaws.com", - "https://egov-s3-patient-data-10bedicu.s3.amazonaws.com", - "http://localhost:4566" - ].join(" "); + env.REACT_CDN_URLS || + [ + "https://egov-s3-facility-10bedicu.s3.amazonaws.com", + "https://egov-s3-patient-data-10bedicu.s3.amazonaws.com", + "http://localhost:4566", + ].join(" ");🧰 Tools
🪛 eslint
[error] 169-169: Insert
··(prettier/prettier)
[error] 170-170: Insert
··(prettier/prettier)
[error] 171-171: Replace
"http://localhost:4566"with··"http://localhost:4566",(prettier/prettier)
[error] 172-172: Insert
··(prettier/prettier)
205-205: Improve formatting of the Zod schema definitionThe
REACT_CDN_URLSschema definition is improperly formatted. Breaking the method chain into multiple lines enhances readability.Apply this diff to enhance readability:
REACT_CDN_URLS: z - .string().optional().transform((val) => val?.split(" ")).pipe(z.array(z.string().url()).optional()).describe("Optional: Space-separated list of CDN URLs") + .string() + .optional() + .transform((val) => val?.split(" ")) + .pipe(z.array(z.string().url()).optional()) + .describe("Optional: Space-separated list of CDN URLs"),🧰 Tools
🪛 eslint
[error] 205-205: Replace
.string().optional().transform((val)·=>·val?.split("·")).pipe(z.array(z.string().url()).optional()).describe("Optional:·Space-separated·list·of·CDN·URLs")with⏎············.string()⏎············.optional()⏎············.transform((val)·=>·val?.split("·"))⏎············.pipe(z.array(z.string().url()).optional())⏎············.describe("Optional:·Space-separated·list·of·CDN·URLs"),(prettier/prettier)
278-280: Align object properties for better readabilityIn the
aliasobject, the properties are not properly aligned. Formatting them consistently improves code readability.Apply this diff to format the aliases:
resolve: { alias: { ...getPluginAliases(), - "@": path2.resolve(__vite_injected_original_dirname2, "./src"), - "@careConfig": path2.resolve(__vite_injected_original_dirname2, "./care.config.ts"), - "@core": path2.resolve(__vite_injected_original_dirname2, "src/") + "@": path2.resolve(__vite_injected_original_dirname2, "./src"), + "@careConfig": path2.resolve( + __vite_injected_original_dirname2, + "./care.config.ts" + ), + "@core": path2.resolve(__vite_injected_original_dirname2, "src/"), } },🧰 Tools
🪛 eslint
[error] 278-278: Replace
__vite_injected_original_dirname2,·"./care.config.ts"with⏎··········__vite_injected_original_dirname2,⏎··········"./care.config.ts",⏎········(prettier/prettier)
[error] 279-279: Insert
,(prettier/prettier)
[error] 280-280: Insert
,(prettier/prettier)
src/components/Common/SearchByMultipleFields.tsx (1)
365-365: Ensure error messages are localizedThe error message displayed to the user is hardcoded as "Invalid Phone Number". To support internationalization, use the
tfunction fromuseTranslationto localize the error message.Apply this diff to localize the error message:
{error && ( <div className="px-2 mb-1 text-xs font-medium tracking-wide transition-opacity duration-300 error-text text-danger-500"> - {error} + {t("invalid_phone_number")} </div> )}
📜 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(3 hunks)src/components/Common/SearchByMultipleFields.tsx(7 hunks)src/components/Facility/FacilityForm.tsx(5 hunks)src/components/Patient/PatientIndex.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(4 hunks)src/components/Users/CreateUserForm.tsx(2 hunks)vite.config.mts.timestamp-1737379816854-14baa337b2fe5.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- public/locale/en.json
🧰 Additional context used
🪛 GitHub Actions: Cypress Tests
src/components/Facility/FacilityForm.tsx
[error] Failed to import '/home/runner/work/care_fe/care_fe/src/src/components/ui/input-phone'. The file or directory does not exist. This appears to be an incorrect import path with a duplicated 'src' directory.
🪛 GitHub Actions: Deploy Care Fe
src/components/Facility/FacilityForm.tsx
[error] Missing dependency: Could not load /app/src/src/components/ui/input-phone. File does not exist.
🪛 Biome (1.9.4)
vite.config.mts.timestamp-1737379816854-14baa337b2fe5.mjs
[error] 157-157: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🪛 eslint
vite.config.mts.timestamp-1737379816854-14baa337b2fe5.mjs
[error] 6-7: Replace fs2·from·"fs";⏎import·{·JSDOM·}·from·"file:///D:/care_fe/node_modules/jsdom/lib/api with {·globSync·}·from·"file:///D:/care_fe/node_modules/glob/dist/esm/index
(prettier/prettier)
[error] 8-8: Replace marked·}·from·"file:///D:/care_fe/node_modules/marked/lib/marked.esm with JSDOM·}·from·"file:///D:/care_fe/node_modules/jsdom/lib/api
(prettier/prettier)
[error] 9-11: Replace createRequire·}·from·"node:module";⏎import·path2·from·"path";⏎import·{·defineConfig,·loadEnv·}·from·"file:///D:/care_fe/node_modules/vite/dist/node/index with marked·}·from·"file:///D:/care_fe/node_modules/marked/lib/marked.esm
(prettier/prettier)
[error] 15-15: Insert ⏎··defineConfig,⏎··loadEnv,⏎}·from·"file:///D:/care_fe/node_modules/vite/dist/node/index.js";⏎import·{
(prettier/prettier)
[error] 16-16: Insert import·fs2·from·"fs";
(prettier/prettier)
[error] 19-19: Replace globSync·}·from·"file:///D:/care_fe/node_modules/glob/dist/esm/index.js with createRequire·}·from·"node:module";⏎import·path2·from·"path
(prettier/prettier)
[error] 20-20: Insert ⏎
(prettier/prettier)
[error] 28-28: Insert ,
(prettier/prettier)
[error] 29-29: Insert ,
(prettier/prettier)
[error] 35-35: Insert ,
(prettier/prettier)
[error] 71-71: Insert ,
(prettier/prettier)
[error] 74-74: Insert ,
(prettier/prettier)
[error] 80-80: Insert ⏎·
(prettier/prettier)
[error] 83-83: Replace "pdfjs-dist/package.json") with ⏎······"pdfjs-dist/package.json",⏎····),
(prettier/prettier)
[error] 86-86: Insert ,
(prettier/prettier)
[error] 92-92: Insert ,
(prettier/prettier)
[error] 111-111: Insert ,
(prettier/prettier)
[error] 120-120: Insert ,
(prettier/prettier)
[error] 128-128: Insert ,
(prettier/prettier)
[error] 136-136: Insert ,
(prettier/prettier)
[error] 142-142: Insert ,
(prettier/prettier)
[error] 148-148: Insert ,
(prettier/prettier)
[error] 161-161: Insert ,
(prettier/prettier)
[error] 162-162: Insert ,
(prettier/prettier)
[error] 168-168: Replace ·env.REACT_CDN_URLS·|| with ⏎····env.REACT_CDN_URLS·||⏎···
(prettier/prettier)
[error] 169-169: Insert ··
(prettier/prettier)
[error] 170-170: Insert ··
(prettier/prettier)
[error] 171-171: Replace "http://localhost:4566" with ··"http://localhost:4566",
(prettier/prettier)
[error] 172-172: Insert ··
(prettier/prettier)
[error] 177-177: Insert ,
(prettier/prettier)
[error] 179-179: Insert ,
(prettier/prettier)
[error] 196-196: Insert ,
(prettier/prettier)
[error] 197-197: Insert ,
(prettier/prettier)
[error] 205-205: Replace .string().optional().transform((val)·=>·val?.split("·")).pipe(z.array(z.string().url()).optional()).describe("Optional:·Space-separated·list·of·CDN·URLs") with ⏎············.string()⏎············.optional()⏎············.transform((val)·=>·val?.split("·"))⏎············.pipe(z.array(z.string().url()).optional())⏎············.describe("Optional:·Space-separated·list·of·CDN·URLs"),
(prettier/prettier)
[error] 206-206: Insert ,
(prettier/prettier)
[error] 212-212: Insert ,
(prettier/prettier)
[error] 213-213: Insert ,
(prettier/prettier)
[error] 214-214: Insert ,
(prettier/prettier)
[error] 222-222: Insert ,
(prettier/prettier)
[error] 223-223: Insert ,
(prettier/prettier)
[error] 224-224: Insert ,
(prettier/prettier)
[error] 227-227: Insert ,
(prettier/prettier)
[error] 236-236: Insert ,
(prettier/prettier)
[error] 239-239: Insert ,
(prettier/prettier)
[error] 251-251: Insert ,
(prettier/prettier)
[error] 256-256: Insert ,
(prettier/prettier)
[error] 262-262: Insert ,
(prettier/prettier)
[error] 268-268: Insert ,
(prettier/prettier)
[error] 269-269: Insert ,
(prettier/prettier)
[error] 270-270: Insert ,
(prettier/prettier)
[error] 271-271: Insert ,
(prettier/prettier)
[error] 272-272: Insert ,
(prettier/prettier)
[error] 278-278: Replace __vite_injected_original_dirname2,·"./care.config.ts" with ⏎··········__vite_injected_original_dirname2,⏎··········"./care.config.ts",⏎········
(prettier/prettier)
[error] 279-279: Insert ,
(prettier/prettier)
[error] 280-280: Insert ,
(prettier/prettier)
[error] 288-288: Insert ,
(prettier/prettier)
[error] 291-291: Insert ,
(prettier/prettier)
[error] 294-294: Insert ,
(prettier/prettier)
[error] 298-298: Insert ,
(prettier/prettier)
[error] 300-300: Insert ,
(prettier/prettier)
[error] 301-301: Insert ,
(prettier/prettier)
[error] 304-306: Replace ⏎··vite_config_default·as·default⏎ with ·vite_config_default·as·default·
(prettier/prettier)
🔇 Additional comments (3)
src/components/Facility/FacilityForm.tsx (1)
79-80: Use translation function for error message.The error message "Invalid phone number" is hardcoded in English.
🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to import '/home/runner/work/care_fe/care_fe/src/src/components/ui/input-phone'. The file or directory does not exist. This appears to be an incorrect import path with a duplicated 'src' directory.
🪛 GitHub Actions: Deploy Care Fe
[error] Missing dependency: Could not load /app/src/src/components/ui/input-phone. File does not exist.
src/components/Patient/PatientRegistration.tsx (2)
269-284: Good implementation of error handling in the query function.The try-catch block with specific error type checking and user feedback is a good practice.
850-852:⚠️ Potential issueAdd safety checks for phone number parsing.
Direct usage of
parsePhoneNumberWithErrorin JSX could cause runtime errors.-!!parsePhoneNumberWithError( - form.watch("phone_number") || "", -).number.toString() +(() => { + try { + return !!parsePhoneNumberWithError( + form.watch("phone_number") || "" + ).number.toString(); + } catch { + return false; + } +})()Likely invalid or redundant comment.
| const { repo, url } = parseRemoteConfig(app); | ||
| console.log(`Configuring Remote Module for ${repo}:`, url); | ||
| return { | ||
| ...acc, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using spread syntax on accumulators in reduce
Using the spread operator ... on accumulators within a .reduce function can lead to performance issues due to increased time complexity (O(n^2)). Consider using an imperative approach or methods like Object.assign to improve performance.
Apply this diff to optimize the code:
function getRemotes(enabledApps) {
if (!enabledApps) return {};
- return enabledApps.split(",").reduce((acc, app) => {
+ const remotes = {};
+ enabledApps.split(",").forEach((app) => {
const { repo, url } = parseRemoteConfig(app);
console.log(`Configuring Remote Module for ${repo}:`, url);
- return {
- ...acc,
- [repo]: {
+ remotes[repo] = {
external: `Promise.resolve("${url}")`,
from: "vite",
externalType: "promise"
- }
- };
- }, {});
+ };
+ });
+ return remotes;
}📝 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.
| ...acc, | |
| function getRemotes(enabledApps) { | |
| if (!enabledApps) return {}; | |
| const remotes = {}; | |
| enabledApps.split(",").forEach((app) => { | |
| const { repo, url } = parseRemoteConfig(app); | |
| console.log(`Configuring Remote Module for ${repo}:`, url); | |
| remotes[repo] = { | |
| external: `Promise.resolve("${url}")`, | |
| from: "vite", | |
| externalType: "promise" | |
| }; | |
| }); | |
| return remotes; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
| import fs2 from "fs"; | ||
| import { JSDOM } from "file:///D:/care_fe/node_modules/jsdom/lib/api.js"; | ||
| import { marked } from "file:///D:/care_fe/node_modules/marked/lib/marked.esm.js"; | ||
| import { createRequire } from "node:module"; | ||
| import path2 from "path"; | ||
| import { defineConfig, loadEnv } from "file:///D:/care_fe/node_modules/vite/dist/node/index.js"; | ||
| import checker from "file:///D:/care_fe/node_modules/vite-plugin-checker/dist/esm/main.js"; | ||
| import { VitePWA } from "file:///D:/care_fe/node_modules/vite-plugin-pwa/dist/index.js"; | ||
| import { viteStaticCopy } from "file:///D:/care_fe/node_modules/vite-plugin-static-copy/dist/index.js"; | ||
| import { z } from "file:///D:/care_fe/node_modules/zod/lib/index.mjs"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Organize imports and fix formatting issues
There are multiple formatting issues and misaligned imports at the beginning of the file. Ensure that the imports are properly organized and formatted according to the project's style guidelines.
Apply this diff to correct the formatting:
// vite.config.mts
-import { ValidateEnv } from "file:///D:/care_fe/node_modules/@julr/vite-plugin-validate-env/dist/index.mjs";
-import federation from "file:///D:/care_fe/node_modules/@originjs/vite-plugin-federation/dist/index.mjs";
-import react from "file:///D:/care_fe/node_modules/@vitejs/plugin-react/dist/index.mjs";
-import DOMPurify from "file:///D:/care_fe/node_modules/dompurify/dist/purify.es.mjs";
-import fs2 from "fs";
-import { JSDOM } from "file:///D:/care_fe/node_modules/jsdom/lib/api.js";
-import { marked } from "file:///D:/care_fe/node_modules/marked/lib/marked.esm.js";
-import { createRequire } from "node:module";
-import path2 from "path";
-import { defineConfig, loadEnv } from "file:///D:/care_fe/node_modules/vite/dist/node/index.js";
-import checker from "file:///D:/care_fe/node_modules/vite-plugin-checker/dist/esm/main.js";
+import { ValidateEnv } from "@julr/vite-plugin-validate-env";
+import federation from "@originjs/vite-plugin-federation";
+import react from "@vitejs/plugin-react";
+import DOMPurify from "dompurify";
+import fs2 from "fs";
+import { JSDOM } from "jsdom";
+import { marked } from "marked";
+import { createRequire } from "node:module";
+import path2 from "path";
+import { defineConfig, loadEnv } from "vite";
+import checker from "vite-plugin-checker";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 6-7: Replace fs2·from·"fs";⏎import·{·JSDOM·}·from·"file:///D:/care_fe/node_modules/jsdom/lib/api with {·globSync·}·from·"file:///D:/care_fe/node_modules/glob/dist/esm/index
(prettier/prettier)
[error] 8-8: Replace marked·}·from·"file:///D:/care_fe/node_modules/marked/lib/marked.esm with JSDOM·}·from·"file:///D:/care_fe/node_modules/jsdom/lib/api
(prettier/prettier)
[error] 9-11: Replace createRequire·}·from·"node:module";⏎import·path2·from·"path";⏎import·{·defineConfig,·loadEnv·}·from·"file:///D:/care_fe/node_modules/vite/dist/node/index with marked·}·from·"file:///D:/care_fe/node_modules/marked/lib/marked.esm
(prettier/prettier)
[error] 15-15: Insert ⏎··defineConfig,⏎··loadEnv,⏎}·from·"file:///D:/care_fe/node_modules/vite/dist/node/index.js";⏎import·{
(prettier/prettier)
| handleSearchChange({ value }); | ||
| setError( | ||
| isValidPhoneNumber(value) | ||
| ? undefined | ||
| : "Invalid Phone Number", | ||
| ); |
There was a problem hiding this comment.
Handle potential exceptions when validating phone numbers
The isValidPhoneNumber function might throw an exception if the input is not in a recognized format. It's safer to wrap the validation in a try-catch block to prevent the application from crashing due to unexpected input.
Apply this diff to handle exceptions:
onValueChange={(value) => {
handleSearchChange({ value });
- setError(
- isValidPhoneNumber(value)
- ? undefined
- : "Invalid Phone Number",
- );
+ try {
+ setError(
+ isValidPhoneNumber(value)
+ ? undefined
+ : "Invalid Phone Number",
+ );
+ } catch (error) {
+ setError("Invalid Phone Number");
+ }
}}Committable suggestion skipped: line range outside the PR's diff.
| .refine(isValidNumber, t("phone_number_validation")) | ||
| .transform((value) => | ||
| parsePhoneNumberWithError(value).number.toString(), | ||
| ), |
There was a problem hiding this comment.
Enhance error handling for phone number parsing.
The direct usage of parsePhoneNumberWithError in the transform function could throw runtime errors if the phone number is invalid. Consider adding a try-catch block.
.transform((value) =>
- parsePhoneNumberWithError(value).number.toString(),
+ {
+ try {
+ return parsePhoneNumberWithError(value).number.toString();
+ } catch (error) {
+ throw new Error(t("invalid_phone_number"));
+ }
+ }
),📝 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) { | |
| throw new Error(t("invalid_phone_number")); | |
| } | |
| } | |
| ), |
| import GovtOrganizationSelector from "@/pages/Organization/components/GovtOrganizationSelector"; | ||
| import { PhoneInput } from "@/src/components/ui/input-phone"; |
There was a problem hiding this comment.
Fix incorrect import path for PhoneInput component.
The import path contains a duplicated 'src' directory which is causing pipeline failures.
-import { PhoneInput } from "@/src/components/ui/input-phone";
+import { PhoneInput } from "@/components/ui/input-phone";📝 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 { PhoneInput } from "@/src/components/ui/input-phone"; | |
| import { PhoneInput } from "@/components/ui/input-phone"; |
🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to import '/home/runner/work/care_fe/care_fe/src/src/components/ui/input-phone'. The file or directory does not exist. This appears to be an incorrect import path with a duplicated 'src' directory.
🪛 GitHub Actions: Deploy Care Fe
[error] Missing dependency: Could not load /app/src/src/components/ui/input-phone. File does not exist.
| updateFacility({ ...data, geo_organization: props.organizationId }); | ||
| } else { | ||
| createFacility(requestData); | ||
| createFacility({ ...data, geo_organization: props.organizationId }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add error handlers to mutation hooks for better error reporting
The form has strong input validation but lacks error handling for API failures. Add onError callbacks to both mutation hooks:
createFacilitymutationupdateFacilitymutation
Example:
onError: (error) => {
toast.error(t("failed_to_create_facility"));
console.error(error);
}🔗 Analysis chain
Consider adding error handling for update and create operations.
The mutation operations directly spread the data object without any validation or sanitization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any error handling patterns in mutation operations
rg -A 5 "mutate.*facility\.(create|update)" --type ts
Length of output: 572
Script:
#!/bin/bash
# Check the mutate utility implementation and form validation
echo "=== Searching for mutate utility implementation ==="
rg -A 10 "export.*mutate.*=.*" --type ts
echo -e "\n=== Checking form validation and error handling in FacilityForm ==="
rg -B 5 -A 10 "const.*FacilityForm.*=.*" src/components/Facility/FacilityForm.tsx
echo -e "\n=== Looking for error handling patterns ==="
rg "onError.*toast\." --type ts
Length of output: 954
Script:
#!/bin/bash
# Search for mutation configuration and form validation
echo "=== Checking mutation hook configuration ==="
rg -B 5 -A 10 "useMutation.*facility" src/components/Facility/FacilityForm.tsx
echo -e "\n=== Looking for form validation setup ==="
rg -B 2 -A 5 "useForm.*FacilityForm" src/components/Facility/FacilityForm.tsx
echo -e "\n=== Checking for error handling patterns ==="
rg -A 5 "onError:|catch\s*\([^)]*\)\s*{" src/components/Facility/FacilityForm.tsx
Length of output: 913
Script:
#!/bin/bash
echo "=== Checking complete FacilityForm implementation ==="
cat src/components/Facility/FacilityForm.tsx
echo -e "\n=== Looking for error handling patterns with fixed regex ==="
rg "onError" src/components/Facility/FacilityForm.tsx
Length of output: 18765
🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to import '/home/runner/work/care_fe/care_fe/src/src/components/ui/input-phone'. The file or directory does not exist. This appears to be an incorrect import path with a duplicated 'src' directory.
🪛 GitHub Actions: Deploy Care Fe
[error] Missing dependency: Could not load /app/src/src/components/ui/input-phone. File does not exist.
|
@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. |
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
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