Revamp Asset Create/Update Workflow using ShadCN, React-Hook-Form, and TanStack Query #9192
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the asset creation workflow, focusing on enhancing form management and validation. Key changes include the integration of Changes
Assessment against linked issues
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
113-147: Consider using a more maintainable type definition.The type definition could be extracted and reused to improve maintainability and reduce duplication.
+type AssetFormValues = { + name: string; + description: string; + location: string; + asset_class: AssetClass | undefined; + is_working: string | undefined; + not_working_reason: string; + serial_number: string; + vendor_name: string; + support_name: string; + support_email: string; + support_phone: string; + qr_code_id: string; + manufacturer: string; + warranty_amc_end_of_validity: any; + last_serviced_on: any; + notes: string; +}; -const [initialValues, setInitialValues] = useState<{...}>({ +const [initialValues, setInitialValues] = useState<AssetFormValues>({
267-313: Optimize form value comparison logic.The current implementation recreates the
currentValuesobject on every render. Consider memoizing it withuseMemoto prevent unnecessary object creation.+const currentValues = useMemo(() => ({ + name, + description, + location, + asset_class, + is_working, + not_working_reason, + serial_number, + vendor_name, + support_name, + support_email, + support_phone, + qr_code_id: qrCodeId, + manufacturer, + warranty_amc_end_of_validity, + last_serviced_on, + notes, +}), [ + name, + description, + location, + asset_class, + is_working, + not_working_reason, + serial_number, + vendor_name, + support_name, + support_email, + support_phone, + qrCodeId, + manufacturer, + warranty_amc_end_of_validity, + last_serviced_on, + notes, +]); -const currentValues = { - name, - description, - // ... other fields -};
288-288: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log("Initial Values " + currentValues);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
1005-1005: LGTM! Good implementation of form submission optimization.
The disabled state on submit buttons effectively prevents unnecessary API calls when no changes are made to the form, which directly addresses the PR objectives.
Also applies to: 1012-1012
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/AssetCreate.tsx (4)
64-88: Consider improving the AssetT interface.The interface has some potential improvements:
- The
notesfield is marked as required (no?), but it's optional in the form.- The
last_servicetype is set toany, which should be avoided for type safety.interface AssetT { - last_service?: any; + last_service?: { + serviced_on?: string; + note?: string; + }; - notes: string; + notes?: string; }
225-263: Enhance form validation for better user experience.The validation logic has some areas for improvement:
- Phone number validation is complex and could be extracted into a separate function
- Email validation could be moved closer to the email field validation
const AssetFormValidator = (form: AssetT): FormErrors<AssetT> => { const errors: Partial<Record<keyof AssetT, FieldError>> = {}; + const validateSupportPhone = (phone?: string): string | undefined => { + if (!phone) return t("field_required"); + const checkTollFree = phone.startsWith("1800"); + const supportPhoneSimple = phone.replace(/[^0-9]/g, "").slice(2); + if (supportPhoneSimple.length !== 10 && !checkTollFree) { + return "Please enter valid phone number"; + } + if ((phone.length < 10 || phone.length > 11) && checkTollFree) { + return "Please enter valid phone number"; + } + return undefined; + }; errors.name = RequiredFieldValidator()(form.name); errors.is_working = form.is_working === undefined ? t("field_required") : undefined; errors.location = !form.location || form.location === "0" || form.location === "" ? "Select a location" : undefined; - if (!form.support_phone) { - errors.support_phone = t("field_required"); - } else { - const checkTollFree = form.support_phone.startsWith("1800"); - // ... rest of phone validation - } + errors.support_phone = validateSupportPhone(form.support_phone); errors.support_email = form.support_email && !validateEmailAddress(form.support_email) ? "Please enter valid email id" : undefined; errors.serviced_on = form.notes && !form.serviced_on ? "Last serviced on date is required with notes" : undefined; return errors; };
710-734: Improve date validation logic.The warranty expiry date validation could be improved:
- The date comparison logic could be simplified using dayjs
- The validation message could be more user-friendly
<TextFormField {...field("warranty_amc_end_of_validity")} label={t("warranty_amc_expiry")} onChange={(event) => { const { value } = event; const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - if (selectedDate.isBefore(today)) { + if (selectedDate.isBefore(dayjs().startOf('day'))) { Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", + msg: "Please select a future date for warranty/AMC expiry", }); } else { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: selectedDate.format("YYYY-MM-DD"), }); } }} type="date" - min={dayjs().format("YYYY-MM-DD")} + min={dayjs().startOf('day').format("YYYY-MM-DD")} />
813-847: Simplify last serviced date validation.Similar to the warranty date validation, the last serviced date validation could be simplified:
- Use dayjs consistently for date comparisons
- Improve error message clarity
<DateFormField {...field("serviced_on")} label={t("last_serviced_on")} className="mt-2" disableFuture value={field("serviced_on").value ? new Date(field("serviced_on").value) : undefined} onChange={(date) => { - const selectedDate = dayjs(date.value).format("YYYY-MM-DD"); - const today = new Date().toLocaleDateString("en-ca"); + const selectedDate = dayjs(date.value); + const today = dayjs(); - if (selectedDate > today) { + if (selectedDate.isAfter(today)) { Notification.Error({ - msg: "Last Serviced date can't be in the future", + msg: "Service date cannot be in the future", }); } else { field("serviced_on").onChange({ name: "serviced_on", - value: selectedDate, + value: selectedDate.format("YYYY-MM-DD"), }); } }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(9 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
139-141: Form state initialization aligns with PR objectives.
The implementation correctly addresses the PR objective by initializing the form state with fetched values, which will help prevent unnecessary API calls when no changes are made.
Also applies to: 197-221
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
225-263: Simplify phone number validation and ensure consistent translationsThe phone number validation logic could be simplified, and error messages should consistently use translations.
Consider this refactor:
if (!form.support_phone) { errors.support_phone = t("field_required"); } else { - const checkTollFree = form.support_phone.startsWith("1800"); - const supportPhoneSimple = form.support_phone - .replace(/[^0-9]/g, "") - .slice(2); - if (supportPhoneSimple.length !== 10 && !checkTollFree) { - errors.support_phone = "Please enter valid phone number"; - } else if ( - (form.support_phone.length < 10 || form.support_phone.length > 11) && - checkTollFree - ) { - errors.support_phone = "Please enter valid phone number"; + const isTollFree = form.support_phone.startsWith("1800"); + const phoneLength = form.support_phone.replace(/[^0-9]/g, "").length; + const isValidLength = isTollFree ? phoneLength >= 10 && phoneLength <= 11 : phoneLength === 10; + if (!isValidLength) { + errors.support_phone = t("please_enter_valid_phone_number"); } } if (form.support_email && !validateEmailAddress(form.support_email)) { - errors.support_email = "Please enter valid email id"; + errors.support_email = t("please_enter_valid_email"); }
Line range hint
294-356: Improve error handling specificityThe error handling in the submission function could be more specific and informative.
Consider this improvement:
- } catch (error) { - // Handle error (optional) - Notification.Error({ - msg: "An error occurred while processing the asset", - }); + } catch (error: unknown) { + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); + Notification.Error({ + msg: t("error_processing_asset", { operation }), + });
748-769: Consolidate date validation logicThe date validation logic is duplicated between warranty and service date handling. Consider extracting this into a reusable function.
Consider creating a utility function:
const validateDate = (date: string, options: { disableFuture?: boolean, disablePast?: boolean, errorMessage: string }) => { const selectedDate = dayjs(date).format("YYYY-MM-DD"); const today = dayjs().format("YYYY-MM-DD"); if (options.disableFuture && selectedDate > today) { Notification.Error({ msg: options.errorMessage }); return false; } if (options.disablePast && selectedDate < today) { Notification.Error({ msg: options.errorMessage }); return false; } return true; };Then use it in both places:
onChange={(event) => { const { value } = event; - const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - - if (selectedDate.isBefore(today)) { - Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", - }); - } else { + if (validateDate(value, { + disablePast: true, + errorMessage: t("warranty_date_past_error") + })) { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: dayjs(value).format("YYYY-MM-DD"), }); } }}Also applies to: 858-876
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(10 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
64-88: LGTM! Well-structured data management
The consolidated state management using the AssetData interface and proper initialization of form state aligns well with the PR objective of preventing unnecessary API calls.
Also applies to: 113-137, 139-140
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
225-263: Refactor validation logic for better maintainabilityConsider these improvements to the validation logic:
- Move phone validation to a separate validator function
- Use the existing
RequiredFieldValidatorfor support phone- Reuse
validateEmailAddressas a validator functionconst AssetFormValidator = (form: AssetData): FormErrors<AssetData> => { const errors: Partial<Record<keyof AssetData, FieldError>> = {}; errors.name = RequiredFieldValidator()(form.name); if (form.is_working === undefined) { errors.is_working = t("field_required"); } if (!form.location || form.location === "0" || form.location === "") { errors.location = t("select_local_body"); } - if (!form.support_phone) { - errors.support_phone = t("field_required"); - } else { - const checkTollFree = form.support_phone.startsWith("1800"); - const supportPhoneSimple = form.support_phone - .replace(/[^0-9]/g, "") - .slice(2); - if (supportPhoneSimple.length !== 10 && !checkTollFree) { - errors.support_phone = "Please enter valid phone number"; - } else if ( - (form.support_phone.length < 10 || form.support_phone.length > 11) && - checkTollFree - ) { - errors.support_phone = "Please enter valid phone number"; - } - } + errors.support_phone = RequiredFieldValidator()(form.support_phone); + if (form.support_phone && !validatePhoneNumber(form.support_phone)) { + errors.support_phone = t("invalid_phone_number"); + } - if (form.support_email && !validateEmailAddress(form.support_email)) { - errors.support_email = "Please enter valid email id"; + if (form.support_email) { + errors.support_email = validateEmailAddress(form.support_email) + ? undefined + : t("invalid_email"); } if (form.notes && !form.serviced_on) { errors.serviced_on = "Last serviced on date is required with notes"; } return errors; };
Line range hint
294-354: Improve error handling in form submissionThe error handling in the submission function could be more specific:
- The catch block should type the error properly
- The error message could be more descriptive based on the operation (create/update)
} catch (error) { - // Handle error (optional) + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); Notification.Error({ - msg: "An error occurred while processing the asset", + msg: `An error occurred while ${operation} the asset. Please try again.`, }); }
745-769: Refactor warranty date validation for consistencyThe warranty date validation could be improved:
- Move validation logic to a common function
- Use consistent date formatting
+const validateFutureDate = (date: string, fieldName: string) => { + const selectedDate = dayjs(date).format("YYYY-MM-DD"); + const today = dayjs().format("YYYY-MM-DD"); + + if (dayjs(selectedDate).isBefore(today)) { + Notification.Error({ + msg: `${fieldName} date can't be in the past`, + }); + return false; + } + return true; +}; <TextFormField {...field("warranty_amc_end_of_validity")} label={t("warranty_amc_expiry")} onChange={(event) => { const { value } = event; - const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - - if (selectedDate.isBefore(today)) { - Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", - }); - } else { + if (validateFutureDate(value, "Warranty / AMC Expiry")) { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: dayjs(value).format("YYYY-MM-DD"), }); } }} type="date" min={dayjs().format("YYYY-MM-DD")} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(10 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
113-137: LGTM! Form initialization implements the PR objectives
The implementation correctly initializes form state and updates it with fetched values, which enables comparison with current values to prevent unnecessary API calls.
Also applies to: 197-221
|
Depends on #8956. Merge only after that PR is merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/AssetCreate.tsx (1)
350-354: Improve error handling specificityThe error handling could be more specific to help users understand what went wrong.
- } catch (error) { - // Handle error (optional) - Notification.Error({ - msg: "An error occurred while processing the asset", - }); + } catch (error) { + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); + Notification.Error({ + msg: `An error occurred while ${operation} the asset. Please try again.`, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(9 hunks)
🔇 Additional comments (2)
src/components/Facility/AssetCreate.tsx (2)
518-539: LGTM! Form implementation meets requirements
The Form component implementation successfully addresses the PR objectives:
- Initializes form state with fetched values
- Properly handles the "Create & Add More" functionality
- Implements validation and error display
64-88:
Fix incorrect type definition for notes field
The notes field in the AssetData interface is marked as required but it's actually optional in the implementation. This is evident from:
- The validation logic only requiring notes when
serviced_onis present - The form treating it as an optional field
Apply this fix:
- notes: string;
+ notes?: string;Likely invalid or redundant comment.
@Jacobjeevan sir, can you please review the final changes? |
Nothing else apart from translations for now 👍 Also, don't need to refer to anyone here as sir/madam. Just refer to us by our names. |
rithviknishad
added
needs review
and removed
question
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
| @@ -180,210 +242,41 @@ const AssetCreate = (props: AssetProps) => { | |||
| ); | |||
|
|
|||
| const assetQuery = useTanStackQueryInstead(routes.getAsset, { | |||
There was a problem hiding this comment.
useTanStackQueryInstead is also a wrapper around useQuery hook right?
I've replaced those with useQuery for GET and useMutation for POST/UPDATE requests.
| className="w-auto p-0" | ||
| align="start" | ||
| > | ||
| <Calendar |
There was a problem hiding this comment.
There's issues with both rendering and behavior for the calendar. If you aren't able to solve it, stick with existing DateFormField for now; we will need to migrate or create a similar component with Shadcn's styles eventually.
There was a problem hiding this comment.
will be working on it..
There was a problem hiding this comment.
There's issues with both rendering and behavior for the calendar. If you aren't able to solve it, stick with existing DateFormField for now; we will need to migrate or create a similar component with Shadcn's styles eventually.
@Jacobjeevan tried to align it properly, but i think its a problem in the template itself, hope they fix it soon🤞
I've switched to good old DateFormField for Expiry and Last Service Date fields wrapped in ShadCN FormControl, please review the changes.
Thank You
Thanks🙌 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
86-152: Improve form submission error handling and state management.
- The error handling in the submit function could be more robust
- The
addMorestate management could be simplifiedconst onSubmit = async (values: FormData) => { if (Object.keys(errors).length > 0) { const firstErrorField = Object.keys(errors)[0] as keyof FormData; form.setFocus(firstErrorField, { shouldSelect: true }); const firstErrorElement = document.querySelector( `[name=${firstErrorField}]`, ); if (firstErrorElement) { firstErrorElement.scrollIntoView({ behavior: "smooth", block: "center", }); } + return; } setLoading(true); + try { const data: any = { // ... data preparation }; if (!assetId) { const res = await createAsset(data); - if (res) { - Notification.Success({ msg: "Asset created successfully" }); - if (addMore) { - form.reset(); - const pageContainer = window.document.getElementById("pages"); - pageContainer?.scroll(0, 0); - setAddMore(false); - } else { - goBack(); - } - } + Notification.Success({ msg: t("asset_created_successfully") }); + if (addMore) { + form.reset(); + window.document.getElementById("pages")?.scroll(0, 0); + } else { + goBack(); + } } else { const res = await updateAsset(data); - if (res) { - Notification.Success({ msg: "Asset updated successfully" }); - goBack(); - } + Notification.Success({ msg: t("asset_updated_successfully") }); + goBack(); } + } catch (error) { + // Error handling is now moved to mutation callbacks + } finally { setLoading(false); } };
153-169: Improve QR code parsing error handling.The error handling in
parseAssetIdcould be more specific and user-friendly.const parseAssetId = (assetUrl: string) => { try { const params = parseQueryParams(assetUrl); const assetId = params.asset || params.assetQR; if (assetId) { form.setValue("qr_code_id", assetId); form.clearErrors("qr_code_id"); setIsScannerActive(false); return; } + throw new Error(t("invalid_qr_code")); } catch (err) { - Notification.Error({ msg: err }); + Notification.Error({ + msg: err instanceof Error ? err.message : t("qr_code_parsing_error"), + }); } - Notification.Error({ msg: "Invalid Asset Id" }); setIsScannerActive(false); };
Line range hint
68-977: Overall component structure looks good with some areas for improvement.The component has been well-refactored to use modern form handling with react-hook-form and Zod. Consider these improvements:
- Add missing translations for form labels
- Enhance error handling in mutations and QR code scanning
- Review Calendar component implementation
- Consider using TypeScript's strict mode to avoid
anytype in form dataThe component structure and organization are clean and maintainable.
📜 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 (1)
src/components/Facility/AssetCreate.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Facility/AssetCreate.tsx (2)
1-57: LGTM! Well-organized imports using absolute paths.
The imports are properly organized and use absolute paths as requested in previous reviews.
901-909: Review Calendar component implementation.
As mentioned in previous reviews, there are issues with the Calendar component's rendering and behavior. Consider using the existing DateFormField component until the Calendar component is properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/Facility/AssetCreate.tsx (2)
132-145: 🛠️ Refactor suggestionAdd error handling to mutations.
The mutations should handle errors explicitly to provide better user feedback.
const { mutateAsync: createAsset } = useMutation({ mutationFn: mutate(routes.createAsset), + onError: (error) => { + Notification.Error({ + msg: t("failed_to_create_asset"), + }); + }, }); const { mutateAsync: updateAsset } = useMutation({ mutationFn: mutate(routes.updateAsset, { pathParams: { external_id: String(assetId) }, }), + onError: (error) => { + Notification.Error({ + msg: t("failed_to_update_asset"), + }); + }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: [assetQueryKey], }); }, });
455-455: 🛠️ Refactor suggestionAdd translations for form labels.
Several form labels are missing translations. Add translation keys for these labels in the en.json file:
- Asset Name
- Asset Class
- Manufacturer
- Customer Support Name
- Customer Support Email
- Vendor Name
- Serial Number
- Last Serviced On
- Notes
Also applies to: 513-515, 690-692, 772-774, 819-821, 841-843, 863-865, 887-889, 943-943
🧹 Nitpick comments (1)
src/components/Facility/AssetCreate.tsx (1)
213-229: Improve error handling in asset ID parsing.The error handling in
parseAssetIdcould be more specific:
- Type the error properly
- Use translations for error messages
try { const params = parseQueryParams(assetUrl); const assetId = params.asset || params.assetQR; if (assetId) { form.setValue("qr_code_id", assetId); form.clearErrors("qr_code_id"); setIsScannerActive(false); return; } - } catch (err) { + } catch (error) { - Notification.Error({ msg: err }); + console.error("Error parsing asset ID:", error); + Notification.Error({ msg: t("error_parsing_asset_id") }); } - Notification.Error({ msg: "Invalid Asset Id" }); + Notification.Error({ msg: t("invalid_asset_id") }); setIsScannerActive(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Facility/AssetCreate.tsx (2)
1-56: Switch to absolute imports consistently.
As mentioned in the past review comments, let's maintain consistency by using absolute imports throughout the file.
For example:
-import { cn } from "@/lib/utils";
+import { cn } from "@/lib/utils";
-import CareIcon, { IconName } from "@/CAREUI/icons/CareIcon";
+import CareIcon, { IconName } from "@/CAREUI/icons/CareIcon";231-253: Well-structured form setup with proper typing and validation!
The form setup follows best practices:
- Uses Zod for type-safe schema validation
- Properly infers types from the schema
- Sets appropriate default values
| const onSubmit = async (values: FormData) => { | ||
| if (Object.keys(errors).length > 0) { | ||
| const firstErrorField = Object.keys(errors)[0] as keyof FormData; | ||
| form.setFocus(firstErrorField, { shouldSelect: true }); | ||
| const firstErrorElement = document.querySelector( | ||
| `[name=${firstErrorField}]`, | ||
| ); | ||
| if (firstErrorElement) { | ||
| firstErrorElement.scrollIntoView({ | ||
| behavior: "smooth", | ||
| block: "center", | ||
| }); | ||
| } | ||
| } | ||
| setLoading(true); | ||
|
|
||
| const data: any = { | ||
| name: values.name, | ||
| asset_type: AssetType.INTERNAL, | ||
| asset_class: values.asset_class || "", | ||
| description: values.description, | ||
| is_working: values.is_working, | ||
| not_working_reason: values.is_working ? "" : values.not_working_reason, | ||
| serial_number: values.serial_number, | ||
| location: values.location, | ||
| vendor_name: values.vendor_name, | ||
| support_name: values.support_name, | ||
| support_email: values.support_email, | ||
| support_phone: values.support_phone?.startsWith("1800") | ||
| ? values.support_phone | ||
| : parsePhoneNumber(String(values.support_phone)), | ||
| qr_code_id: values.qr_code_id !== "" ? values.qr_code_id : null, | ||
| manufacturer: values.manufacturer, | ||
| warranty_amc_end_of_validity: values.warranty_amc_end_of_validity | ||
| ? dateQueryString(values.warranty_amc_end_of_validity) | ||
| : null, | ||
| }; | ||
|
|
||
| if (values.serviced_on) { | ||
| data["last_serviced_on"] = dateQueryString(values.serviced_on); | ||
| data["note"] = values.note ?? ""; | ||
| } | ||
|
|
||
| if (!assetId) { | ||
| const res = await createAsset(data); | ||
| if (res) { | ||
| Notification.Success({ msg: "Asset created successfully" }); | ||
| if (addMore) { | ||
| form.reset(); | ||
| const pageContainer = window.document.getElementById("pages"); | ||
| pageContainer?.scroll(0, 0); | ||
| setAddMore(false); | ||
| } else { | ||
| goBack(); | ||
| } | ||
| } | ||
| setLoading(false); | ||
| } else { | ||
| const res = await updateAsset(data); | ||
| if (res) { | ||
| Notification.Success({ msg: "Asset updated successfully" }); | ||
| goBack(); | ||
| } | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve form submission handling.
A few suggestions for improvement:
- Use translations for success messages
- Add error handling for the else case in update flow
- Consider using a phone number validator utility
if (!assetId) {
const res = await createAsset(data);
if (res) {
- Notification.Success({ msg: "Asset created successfully" });
+ Notification.Success({ msg: t("asset_created_successfully") });
if (addMore) {
form.reset();
const pageContainer = window.document.getElementById("pages");
pageContainer?.scroll(0, 0);
setAddMore(false);
} else {
goBack();
}
+ } else {
+ Notification.Error({ msg: t("failed_to_create_asset") });
}
setLoading(false);
} else {
const res = await updateAsset(data);
if (res) {
- Notification.Success({ msg: "Asset updated successfully" });
+ Notification.Success({ msg: t("asset_updated_successfully") });
goBack();
+ } else {
+ Notification.Error({ msg: t("failed_to_update_asset") });
}
setLoading(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.
| const onSubmit = async (values: FormData) => { | |
| if (Object.keys(errors).length > 0) { | |
| const firstErrorField = Object.keys(errors)[0] as keyof FormData; | |
| form.setFocus(firstErrorField, { shouldSelect: true }); | |
| const firstErrorElement = document.querySelector( | |
| `[name=${firstErrorField}]`, | |
| ); | |
| if (firstErrorElement) { | |
| firstErrorElement.scrollIntoView({ | |
| behavior: "smooth", | |
| block: "center", | |
| }); | |
| } | |
| } | |
| setLoading(true); | |
| const data: any = { | |
| name: values.name, | |
| asset_type: AssetType.INTERNAL, | |
| asset_class: values.asset_class || "", | |
| description: values.description, | |
| is_working: values.is_working, | |
| not_working_reason: values.is_working ? "" : values.not_working_reason, | |
| serial_number: values.serial_number, | |
| location: values.location, | |
| vendor_name: values.vendor_name, | |
| support_name: values.support_name, | |
| support_email: values.support_email, | |
| support_phone: values.support_phone?.startsWith("1800") | |
| ? values.support_phone | |
| : parsePhoneNumber(String(values.support_phone)), | |
| qr_code_id: values.qr_code_id !== "" ? values.qr_code_id : null, | |
| manufacturer: values.manufacturer, | |
| warranty_amc_end_of_validity: values.warranty_amc_end_of_validity | |
| ? dateQueryString(values.warranty_amc_end_of_validity) | |
| : null, | |
| }; | |
| if (values.serviced_on) { | |
| data["last_serviced_on"] = dateQueryString(values.serviced_on); | |
| data["note"] = values.note ?? ""; | |
| } | |
| if (!assetId) { | |
| const res = await createAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: "Asset created successfully" }); | |
| if (addMore) { | |
| form.reset(); | |
| const pageContainer = window.document.getElementById("pages"); | |
| pageContainer?.scroll(0, 0); | |
| setAddMore(false); | |
| } else { | |
| goBack(); | |
| } | |
| } | |
| setLoading(false); | |
| } else { | |
| const res = await updateAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: "Asset updated successfully" }); | |
| goBack(); | |
| } | |
| setLoading(false); | |
| } | |
| }; | |
| const onSubmit = async (values: FormData) => { | |
| if (Object.keys(errors).length > 0) { | |
| const firstErrorField = Object.keys(errors)[0] as keyof FormData; | |
| form.setFocus(firstErrorField, { shouldSelect: true }); | |
| const firstErrorElement = document.querySelector( | |
| `[name=${firstErrorField}]`, | |
| ); | |
| if (firstErrorElement) { | |
| firstErrorElement.scrollIntoView({ | |
| behavior: "smooth", | |
| block: "center", | |
| }); | |
| } | |
| } | |
| setLoading(true); | |
| const data: any = { | |
| name: values.name, | |
| asset_type: AssetType.INTERNAL, | |
| asset_class: values.asset_class || "", | |
| description: values.description, | |
| is_working: values.is_working, | |
| not_working_reason: values.is_working ? "" : values.not_working_reason, | |
| serial_number: values.serial_number, | |
| location: values.location, | |
| vendor_name: values.vendor_name, | |
| support_name: values.support_name, | |
| support_email: values.support_email, | |
| support_phone: values.support_phone?.startsWith("1800") | |
| ? values.support_phone | |
| : parsePhoneNumber(String(values.support_phone)), | |
| qr_code_id: values.qr_code_id !== "" ? values.qr_code_id : null, | |
| manufacturer: values.manufacturer, | |
| warranty_amc_end_of_validity: values.warranty_amc_end_of_validity | |
| ? dateQueryString(values.warranty_amc_end_of_validity) | |
| : null, | |
| }; | |
| if (values.serviced_on) { | |
| data["last_serviced_on"] = dateQueryString(values.serviced_on); | |
| data["note"] = values.note ?? ""; | |
| } | |
| if (!assetId) { | |
| const res = await createAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: t("asset_created_successfully") }); | |
| if (addMore) { | |
| form.reset(); | |
| const pageContainer = window.document.getElementById("pages"); | |
| pageContainer?.scroll(0, 0); | |
| setAddMore(false); | |
| } else { | |
| goBack(); | |
| } | |
| } else { | |
| Notification.Error({ msg: t("failed_to_create_asset") }); | |
| } | |
| setLoading(false); | |
| } else { | |
| const res = await updateAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: t("asset_updated_successfully") }); | |
| goBack(); | |
| } else { | |
| Notification.Error({ msg: t("failed_to_update_asset") }); | |
| } | |
| setLoading(false); | |
| } | |
| }; |
rajku-dev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Facility/AssetCreate.tsx (3)
1-48: LGTM! Well-organized imports and type definitions.
The code follows best practices with:
- Consistent use of absolute imports
- Clear organization of imports by category
- Proper type definitions for props and form data
124-137: 🛠️ Refactor suggestion
Add error handling to mutations.
The mutations should handle errors explicitly to provide better user feedback.
const { mutateAsync: createAsset } = useMutation({
mutationFn: mutate(routes.createAsset),
+ onError: (error) => {
+ Notification.Error({
+ msg: t("failed_to_create_asset"),
+ });
+ },
});
const { mutateAsync: updateAsset } = useMutation({
mutationFn: mutate(routes.updateAsset, {
pathParams: { external_id: String(assetId) },
}),
+ onError: (error) => {
+ Notification.Error({
+ msg: t("failed_to_update_asset"),
+ });
+ },
onSuccess: () => {
queryClient.invalidateQueries({
queryKey: [assetQueryKey],
});
},
});Likely invalid or redundant comment.
97-122:
Add null check for location_object access.
The code directly accesses asset.location_object.id without checking if location_object exists.
useEffect(() => {
if (assetId && assetQuery.data && assetQuery.data.data) {
const asset = assetQuery.data.data;
form.reset({
name: asset.name,
- location: asset.location_object.id,
+ location: asset.location_object?.id || "",
asset_class: asset.asset_class,
// ... rest of the fields
});
}
}, [assetQuery.data]);Likely invalid or redundant comment.
| Asset Name{" "} | ||
| <span className="text-red-500">*</span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add translations for form labels.
Several form labels are hardcoded in English. Add translation keys for consistency.
Examples of labels needing translation:
- "Asset Name"
- "Location"
- "Asset Class"
- "Description"
- "Why the asset is not working?"
- "QR Code ID"
- "Manufacturer"
- "Warranty / AMC Expiry"
- "Customer Support Name"
- "Customer Support Phone"
- "Customer Support Email"
- "Vendor Name"
- "Serial Number"
- "Last Serviced On Date"
- "Notes"
Add corresponding keys to the translation files and use them with the t() function.
Also applies to: 476-477, 515-516, 559-560, 625-626, 664-665, 700-701, 726-727, 755-756, 781-782, 808-809, 834-835, 860-861, 888-889, 917-917
| if (!assetId) { | ||
| const res = await createAsset(data); | ||
| if (res) { | ||
| Notification.Success({ msg: "Asset created successfully" }); | ||
| if (addMore) { | ||
| form.reset(); | ||
| const pageContainer = window.document.getElementById("pages"); | ||
| pageContainer?.scroll(0, 0); | ||
| setAddMore(false); | ||
| } else { | ||
| goBack(); | ||
| } | ||
| } | ||
| setLoading(false); | ||
| } else { | ||
| const res = await updateAsset(data); | ||
| if (res) { | ||
| Notification.Success({ msg: "Asset updated successfully" }); | ||
| goBack(); | ||
| } | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use translations for success messages.
Success notifications should use translated messages for consistency.
if (!assetId) {
const res = await createAsset(data);
if (res) {
- Notification.Success({ msg: "Asset created successfully" });
+ Notification.Success({ msg: t("asset_created_successfully") });
if (addMore) {
// ... rest of the code
}
}
} else {
const res = await updateAsset(data);
if (res) {
- Notification.Success({ msg: "Asset updated successfully" });
+ Notification.Success({ msg: t("asset_updated_successfully") });
goBack();
}
}📝 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.
| if (!assetId) { | |
| const res = await createAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: "Asset created successfully" }); | |
| if (addMore) { | |
| form.reset(); | |
| const pageContainer = window.document.getElementById("pages"); | |
| pageContainer?.scroll(0, 0); | |
| setAddMore(false); | |
| } else { | |
| goBack(); | |
| } | |
| } | |
| setLoading(false); | |
| } else { | |
| const res = await updateAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: "Asset updated successfully" }); | |
| goBack(); | |
| } | |
| setLoading(false); | |
| } | |
| if (!assetId) { | |
| const res = await createAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: t("asset_created_successfully") }); | |
| if (addMore) { | |
| form.reset(); | |
| const pageContainer = window.document.getElementById("pages"); | |
| pageContainer?.scroll(0, 0); | |
| setAddMore(false); | |
| } else { | |
| goBack(); | |
| } | |
| } | |
| setLoading(false); | |
| } else { | |
| const res = await updateAsset(data); | |
| if (res) { | |
| Notification.Success({ msg: t("asset_updated_successfully") }); | |
| goBack(); | |
| } | |
| setLoading(false); | |
| } |
|
All fields look pretty similar to the original one now
|
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
"assets" is no longer present in new architecture as of now. we can rethink once it's brought back. |
Proposed Changes
issue_9198_fix.mp4
-Initialized form state using 'fetchedValues' to pre-fill the form with asset data.
-Introduced a mechanism to compare current form values with initial values and enable/disable the submit button based on changes made by the user in form.
-Used a useRef (isInitialLoad.current) to skip comparisons during the initial load and avoid unnecessary state updates.
-Once the initial values are set, this flag is updated to ensure further state checks and updates are triggered properly to enable or disable the 'Update' button accordingly.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Improvements