Add Deletion Functionality to Locations #10571
Conversation
WalkthroughThis pull request updates the facility location management by passing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocationCard
participant AlertDialog
participant API
participant QueryClient
User->>LocationCard: Click "Delete" button
LocationCard->>AlertDialog: Open confirmation dialog
User->>AlertDialog: Confirm deletion
AlertDialog->>LocationCard: Send confirmation
LocationCard->>API: Call delete endpoint (facilityId, locationId)
API-->>LocationCard: Acknowledge deletion
LocationCard->>QueryClient: Invalidate location query
LocationCard->>User: Display success toast
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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. |
|
Currently only locations that do not have children can be deleted. This is to prevent wanted locations from being deleted. Please Let me know if this approach should be modified. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
134-136: Add accessibility label to delete button.The delete button should have an aria-label to improve accessibility for screen readers.
- <Button variant="destructive" onClick={() => onDelete?.(location)}> + <Button + variant="destructive" + onClick={() => onDelete?.(location)} + aria-label={t("delete_location")} + >src/pages/Facility/settings/locations/LocationList.tsx (1)
78-83: Enhance error handling to cover network errors.The error handling only processes API errors. Consider handling network errors and other potential failure cases.
onError: (error) => { - const errorData = error.cause as { errors: { msg: string }[] }; - errorData.errors.forEach((er) => { - toast.error(er.msg); - }); + if (error.cause && 'errors' in error.cause) { + const errorData = error.cause as { errors: { msg: string }[] }; + errorData.errors.forEach((er) => { + toast.error(er.msg); + }); + } else { + toast.error(t("failed_to_delete_location")); + } },src/pages/Facility/settings/locations/LocationView.tsx (1)
99-128: Consider extracting shared deletion logic into a custom hook.The deletion logic is duplicated between LocationList and LocationView components. Consider creating a custom hook like
useLocationDeleteto share this functionality.Example implementation:
function useLocationDelete(facilityId: string, options?: { parentId?: string, onSuccess?: () => void }) { const queryClient = useQueryClient(); return useMutation({ mutationFn: ({ locationId }: { locationId: string }) => mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: locationId }, })({ facilityId, locationId }), onSuccess: () => { const queryKey = options?.parentId ? ["locations", facilityId, options.parentId, "children"] : ["locations", facilityId]; queryClient.invalidateQueries({ queryKey }); toast.success("Location removed successfully"); options?.onSuccess?.(); }, onError: (error) => { if (error.cause && 'errors' in error.cause) { const errorData = error.cause as { errors: { msg: string }[] }; errorData.errors.forEach((er) => { toast.error(er.msg); }); } else { toast.error(t("failed_to_delete_location")); } }, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pages/Facility/settings/locations/LocationList.tsx(5 hunks)src/pages/Facility/settings/locations/LocationView.tsx(6 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)src/types/location/locationApi.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/types/location/locationApi.ts (1)
31-35: LGTM! The delete endpoint follows RESTful conventions.The delete endpoint is well-defined with appropriate path parameters and return type.
src/pages/Facility/settings/locations/LocationView.tsx (1)
122-127: Enhance error handling to cover network errors.Similar to LocationList, the error handling should be improved to handle network errors and other failure cases.
Yup! Backend also rejects it anyways |
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 (3)
src/pages/Facility/settings/locations/LocationList.tsx(1 hunks)src/pages/Facility/settings/locations/LocationView.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Facility/settings/locations/LocationView.tsx
- src/pages/Facility/settings/locations/LocationList.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
43-48: LGTM! Props interface update is well-structured.The addition of the optional
facilityIdprop aligns with the component's new deletion functionality while maintaining backward compatibility.
165-166: LGTM! Layout changes are well-structured.The use of
justify-betweenfor button placement creates a clean and balanced layout, with destructive actions (delete) on the left and navigation (view details) on the right.Also applies to: 204-213
src/pages/Facility/settings/locations/components/LocationCard.tsx
Outdated
Show resolved
Hide resolved
| <AlertDialog> | ||
| <AlertDialogTrigger asChild> | ||
| <Button | ||
| className={cn(buttonVariants({ variant: "destructive" }))} | ||
| > | ||
| <CareIcon icon="l-trash" /> | ||
| </Button> | ||
| </AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle> | ||
| {t("remove")} {location.name} | ||
| </AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t("are_you_sure_want_to_delete", { | ||
| name: location.name, | ||
| })} | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> | ||
| <AlertDialogAction | ||
| onClick={() => | ||
| removeLocation({ | ||
| pathParams: { | ||
| facility_id: facilityId, | ||
| id: location.id, | ||
| }, | ||
| }) | ||
| } | ||
| className={cn(buttonVariants({ variant: "destructive" }))} | ||
| > | ||
| {t("remove")} | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> |
There was a problem hiding this comment.
Prevent deletion of locations with children.
Based on the PR comments, locations with children should not be deletable. The UI should reflect this restriction.
Add a check for location.has_children and disable the delete button accordingly:
- <AlertDialog>
+ {!location.has_children ? (
+ <AlertDialog>
<AlertDialogTrigger asChild>
<Button
className={cn(buttonVariants({ variant: "destructive" }))}
>
<CareIcon icon="l-trash" />
</Button>
</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>
{t("remove")} {location.name}
</AlertDialogTitle>
<AlertDialogDescription>
{t("are_you_sure_want_to_delete", {
name: location.name,
})}
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel>{t("cancel")}</AlertDialogCancel>
<AlertDialogAction
onClick={() =>
removeLocation({
pathParams: {
facility_id: facilityId,
id: location.id,
},
})
}
className={cn(buttonVariants({ variant: "destructive" }))}
>
{t("remove")}
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
+ ) : (
+ <Button
+ className={cn(buttonVariants({ variant: "destructive" }))}
+ disabled
+ title={t("cannot_delete_location_with_children")}
+ >
+ <CareIcon icon="l-trash" />
+ </Button>
+ )}📝 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.
| <AlertDialog> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| className={cn(buttonVariants({ variant: "destructive" }))} | |
| > | |
| <CareIcon icon="l-trash" /> | |
| </Button> | |
| </AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle> | |
| {t("remove")} {location.name} | |
| </AlertDialogTitle> | |
| <AlertDialogDescription> | |
| {t("are_you_sure_want_to_delete", { | |
| name: location.name, | |
| })} | |
| </AlertDialogDescription> | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> | |
| <AlertDialogAction | |
| onClick={() => | |
| removeLocation({ | |
| pathParams: { | |
| facility_id: facilityId, | |
| id: location.id, | |
| }, | |
| }) | |
| } | |
| className={cn(buttonVariants({ variant: "destructive" }))} | |
| > | |
| {t("remove")} | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| {!location.has_children ? ( | |
| <AlertDialog> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| className={cn(buttonVariants({ variant: "destructive" }))} | |
| > | |
| <CareIcon icon="l-trash" /> | |
| </Button> | |
| </AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle> | |
| {t("remove")} {location.name} | |
| </AlertDialogTitle> | |
| <AlertDialogDescription> | |
| {t("are_you_sure_want_to_delete", { | |
| name: location.name, | |
| })} | |
| </AlertDialogDescription> | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> | |
| <AlertDialogAction | |
| onClick={() => | |
| removeLocation({ | |
| pathParams: { | |
| facility_id: facilityId, | |
| id: location.id, | |
| }, | |
| }) | |
| } | |
| className={cn(buttonVariants({ variant: "destructive" }))} | |
| > | |
| {t("remove")} | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| ) : ( | |
| <Button | |
| className={cn(buttonVariants({ variant: "destructive" }))} | |
| disabled | |
| title={t("cannot_delete_location_with_children")} | |
| > | |
| <CareIcon icon="l-trash" /> | |
| </Button> | |
| )} |
There was a problem hiding this comment.
☝️ @DonXavierdev Don't show the trash button if location has children
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <AlertDialog> | ||
| <AlertDialogTrigger asChild> | ||
| <Button | ||
| className={cn(buttonVariants({ variant: "destructive" }))} | ||
| > | ||
| <CareIcon icon="l-trash" /> | ||
| </Button> | ||
| </AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle> | ||
| {t("remove")} {location.name} | ||
| </AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t("are_you_sure_want_to_delete", { | ||
| name: location.name, | ||
| })} | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> | ||
| <AlertDialogAction | ||
| onClick={() => | ||
| removeLocation({ | ||
| pathParams: { | ||
| facility_id: facilityId, | ||
| id: location.id, | ||
| }, | ||
| }) | ||
| } | ||
| className={cn(buttonVariants({ variant: "destructive" }))} | ||
| > | ||
| {t("remove")} | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> |
There was a problem hiding this comment.
☝️ @DonXavierdev Don't show the trash button if location has children
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
59-69:⚠️ Potential issueAdd error handling for failed deletions.
While the success case is well-handled with query invalidation and toast notification, there's no error handling for failed deletions.
Add error handling to provide feedback when deletion fails:
const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success("Location removed successfully"); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to remove location"); + }, });
🧹 Nitpick comments (2)
src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
67-67: Internationalize the success message.The success message is hardcoded. Use the translation function to maintain consistency with the rest of the internationalized text.
- toast.success("Location removed successfully"); + toast.success(t("location_removed_successfully"));
170-174: Improve accessibility for the delete button.The delete button lacks proper accessibility attributes for screen readers.
<Button className={cn(buttonVariants({ variant: "destructive" }))} + aria-label={t("delete_location", { name: location.name })} > - <CareIcon icon="l-trash" /> + <CareIcon icon="l-trash" aria-hidden="true" /> + <span className="sr-only">{t("delete_location", { name: location.name })}</span> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
1-48: LGTM! Import statements and interface changes are well-organized.The new imports and Props interface update are correctly implemented to support the delete functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
59-69:⚠️ Potential issueAdd error and loading state handling to the deletion mutation.
While the success case is well-handled, the mutation implementation needs error handling and loading state management.
+ const [isDeleting, setIsDeleting] = useState(false); const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success(t("location_removed_successfully")); + setIsDeleting(false); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : t("failed_to_remove_location")); + setIsDeleting(false); + }, + onMutate: () => { + setIsDeleting(true); + }, });
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
167-205: Add loading state to the delete button and enhance dialog text.The delete button implementation needs loading state indication and more descriptive confirmation text.
{!location.has_children && ( <AlertDialog> <AlertDialogTrigger asChild> <Button className={cn(buttonVariants({ variant: "destructive" }))} + disabled={isDeleting} > - <CareIcon icon="l-trash" /> + {isDeleting ? ( + <CareIcon icon="l-spinner" className="animate-spin" /> + ) : ( + <CareIcon icon="l-trash" /> + )} </Button> </AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle> {t("remove")} {location.name} </AlertDialogTitle> <AlertDialogDescription> - {t("are_you_sure_want_to_delete", { - name: location.name, - })} + {t("location_delete_confirmation", { + name: location.name, + type: getLocationFormLabel(location.form), + })} </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> <AlertDialogAction onClick={() => removeLocation({ pathParams: { facility_id: facilityId, id: location.id, }, }) } className={cn(buttonVariants({ variant: "destructive" }))} + disabled={isDeleting} > - {t("remove")} + {isDeleting ? t("removing") : t("remove")} </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
public/locale/en.json (1)
1254-1254: New Localization Entry for Delete ConfirmationThe newly added key
"location_removed_successfully": "Location removed successfully"fits well with the new deletion functionality. It follows the existing pattern in the file and provides clear feedback upon successful deletion of a location.src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
1-1: LGTM! Well-organized imports and type definitions.The new imports are logically grouped and all necessary dependencies for the deletion functionality are included. The Props interface is correctly updated with the optional facilityId.
Also applies to: 18-18, 22-34, 36-36, 39-41, 47-47
src/pages/Facility/settings/locations/components/LocationCard.tsx
Outdated
Show resolved
Hide resolved
|
Note: Check whether a location that is currently in use should be restricted from being deleted. |
@rithviknishad Can you check if this is backend issue |
@DonXavierdev yes this is a backend issue. |
@nihal467 What should i do here . should i remove the delete button when location has an active label ? |
…o issues/10551/Delete-Button-Locations
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
59-69: 🛠️ Refactor suggestionAdd error handling for failed deletions.
While the success case is well-handled with query invalidation and toast notification, there's no error handling for failed deletions.
Add error handling to provide feedback when deletion fails:
const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success(t("location_removed_successfully")); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : t("failed_to_remove_location")); + }, });
🧹 Nitpick comments (2)
src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
170-172: Use destructive variant for delete button.The delete button should visually communicate its destructive nature. The current "white" variant doesn't sufficiently indicate that this is a destructive action.
- <Button className={cn(buttonVariants({ variant: "white" }))}> + <Button className={cn(buttonVariants({ variant: "destructive" }))}> <CareIcon icon="l-trash" /> </Button>
189-194: Consider optimizing the removeLocation call.The pathParams are redundant as they are already defined in the mutationFn. You can simplify the onClick handler.
- onClick={() => - removeLocation({ - pathParams: { - facility_id: facilityId, - id: location.id, - }, - }) - } + onClick={() => removeLocation()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(1 hunks)src/pages/Facility/settings/locations/LocationView.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)src/types/location/locationApi.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/Facility/settings/locations/LocationView.tsx
- src/types/location/locationApi.ts
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/pages/Facility/settings/locations/components/LocationCard.tsx (5)
167-203: The delete button implementation correctly prevents deletion of locations with children.The implementation appropriately hides the delete button for locations with children, aligning with the requirements discussed in the PR comments.
47-47: Make facilityId prop non-optional.Based on the previous review comments, since the LocationCard is only used on two pages that both provide the facilityId, it should be a required prop as implemented.
1-41: Imports are appropriate for the new functionality.The added imports for useMutation, AlertDialog components, and CareIcon are necessary for implementing the deletion functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
166-166: Layout change from flex-end to justify-between is appropriate.The change from right-aligned to justify-between positioning accommodates the new delete button well.
67-67:✅ Verification successful
Verify the translation key exists.
Ensure that the "location_removed_successfully" translation key exists in all relevant locale files to avoid displaying untranslated text to users.
🏁 Script executed:
#!/bin/bash # Check if the translation key exists in the locale files grep -r "location_removed_successfully" public/locale/Length of output: 144
Translation Key Confirmed
I verified that the translation key
"location_removed_successfully"exists in the English locale file (public/locale/en.json). If your project later supports additional locales, please ensure the same key is present there to prevent any untranslated text.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
1-1: 🛠️ Refactor suggestionAdd error handling for deletion failures.
The code implements the deletion functionality but lacks error handling when the API call fails. Users would not receive any feedback if the deletion encounters an error.
Add error handling to provide feedback when deletion fails:
const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success(t("location_removed_successfully")); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : t("failed_to_remove_location")); + }, });Also applies to: 18-18, 57-69
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
170-172: Use destructive variant for delete button.The delete button currently uses the "white" variant which doesn't visually communicate its destructive nature. Consider using the "destructive" variant to make it clear to users that this is a deletion action.
-<Button className={cn(buttonVariants({ variant: "white" }))}> +<Button className={cn(buttonVariants({ variant: "destructive", size: "icon" }))}> <CareIcon icon="l-trash" /> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (7)
src/pages/Facility/settings/locations/components/LocationCard.tsx (7)
47-47: Update the facilityId prop to be required.Based on the previous comment thread, it was confirmed that this component is only used in locations where facilityId is available, so making it required (non-optional) is appropriate.
167-203: Conditional rendering for delete button is correctly implemented.The code correctly implements the conditions from the PR requirements:
- It prevents deletion of locations with children (!location.has_children)
- It prevents deletion of locations currently in use (!location.current_encounter)
This implementation aligns with both frontend and backend requirements as discussed in the PR comments.
165-166: Change to justify-between is appropriate for the new layout.The modification to use
justify-betweeninstead of the previous layout is appropriate for the new UI that includes both the delete button and view details button.
50-55: Update is consistent with the component's usage.The addition of facilityId to the component parameters is consistent with the described changes in the AI summary, where this prop is now passed from the LocationView component.
36-36: Import change to include buttonVariants is appropriate.The import now includes buttonVariants which is used for styling buttons in the AlertDialog, making this a necessary and appropriate change.
67-67:✅ Verification successful
Verify translation key exists.
Ensure that the "location_removed_successfully" translation key exists in the localization files for all supported languages.
🏁 Script executed:
#!/bin/bash # Check for the translation key in localization files echo "Checking for 'location_removed_successfully' translation key..." # Look for the key in locale files fd -e json -t f "en.json" | xargs rg -l "location_removed_successfully"Length of output: 234
Translation Key Verified
The translation keylocation_removed_successfullywas successfully located inpublic/locale/en.json. Please ensure that if additional locales are supported (e.g., French, Spanish, etc.), this key is also defined in their corresponding localization files.
22-22:❓ Verification inconclusive
Verify CareIcon integration.
Ensure that the CareIcon component correctly renders the trash icon. Verify that the "l-trash" icon exists in the icon library and displays as expected.
Also applies to: 170-172
🏁 Script executed:
#!/bin/bash # Check if the l-trash icon is defined in the icon library echo "Checking for l-trash icon in the CareIcon component..." # Look for l-trash in icon definitions fd -t f "CareIcon" | xargs rg -l "l-trash"Length of output: 159
Attention: Verify the l-trash Icon Integration in CareIcon
- Automated scans did not return any evidence of an
l-trashdefinition in the CareIcon component.- Please manually verify that the icon library includes a definition for
l-trashso that the CareIcon component correctly renders the trash icon.- Confirm the integration in both the file
src/pages/Facility/settings/locations/components/LocationCard.tsxat line 22 and at lines 170–172.
|
not followed merge checklist |
|
@DonXavierdev update the PR's body |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
59-69: 🛠️ Refactor suggestionAdd error handling for failed deletions.
While the success case is well-handled with query invalidation and toast notification, there's no error handling for failed deletions.
Add error handling to provide feedback when deletion fails:
const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success(t("location_removed_successfully")); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to remove location"); + }, });
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
167-209: Implementation correctly restricts deletion.The code properly implements restrictions on location deletion based on PR requirements - locations with children or currently in use cannot be deleted.
Minor improvements:
- The
Buttonvariant is "white" but would be more consistent with destructive actions if changed to "destructive":<Button - className={cn(buttonVariants({ variant: "white" }))} + className={cn(buttonVariants({ variant: "destructive" }))} > <CareIcon icon="l-trash" /> </Button>
- The unnecessary div wrapper can be removed:
- {!location.has_children && !location.current_encounter && ( - <div> - <AlertDialog> + {!location.has_children && !location.current_encounter && ( + <AlertDialog> <AlertDialogTrigger asChild> <Button className={cn(buttonVariants({ variant: "white" }))} > <CareIcon icon="l-trash" /> </Button> </AlertDialogTrigger> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle> {t("remove")} {location.name} </AlertDialogTitle> <AlertDialogDescription> {t("are_you_sure_want_to_delete", { name: location.name, })} </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel>{t("cancel")}</AlertDialogCancel> <AlertDialogAction onClick={() => removeLocation({ pathParams: { facility_id: facilityId, id: location.id, }, }) } className={cn( buttonVariants({ variant: "destructive" }), )} > {t("remove")} </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> - </AlertDialog> - </div> + </AlertDialog> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: CodeQL-Build
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
210-220: View details button correctly positioned.The view details button is properly positioned with
ml-autoto ensure it appears on the right side of the flex container, maintaining good UI layout when the delete button is not present.
47-47: Correctly added facilityId parameter.The facilityId parameter is properly defined as a required string in the Props interface, which is appropriate since it's needed for the delete operation.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…o issues/10551/Delete-Button-Locations
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
46-56: 🛠️ Refactor suggestionAdd error handling for failed deletions.
While the success case is well-handled with query invalidation and toast notification, there's no error handling for failed deletions. Users won't receive feedback if the location removal fails.
const { mutate: removeLocation } = useMutation({ mutationFn: mutate(locationApi.delete, { pathParams: { facility_id: facilityId, id: location.id }, }), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["locations", facilityId], }); toast.success(t("location_removed_successfully")); }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : t("failed_to_remove_location")); + }, });
🧹 Nitpick comments (4)
src/pages/Facility/settings/locations/components/LocationCard.tsx (4)
124-128: Use consistent variant for destructive actions.The delete button uses the "white" variant, but it's a destructive action. For better UX, consider using the "destructive" variant or at least a consistent visual style for all delete-related buttons.
<Button - className={cn(buttonVariants({ variant: "white" }))} + className={cn(buttonVariants({ variant: "destructive" }))} > <CareIcon icon="l-trash" /> </Button>
127-128: Improve accessibility for icon-only buttons.The delete button only contains an icon without text, which might not be accessible to all users. Add an aria-label for screen readers.
<Button className={cn(buttonVariants({ variant: "white" }))} + aria-label={t("remove_location")} > <CareIcon icon="l-trash" /> </Button>
144-151: Optimize onClick handler with useCallback.The onClick handler is recreated on every render. Consider using useCallback to optimize performance, especially for complex UIs with many cards.
+const handleDelete = useCallback(() => { + removeLocation({ + pathParams: { + facility_id: facilityId, + id: location.id, + }, + }); +}, [facilityId, location.id, removeLocation]); // Then in the JSX: <AlertDialogAction - onClick={() => - removeLocation({ - pathParams: { - facility_id: facilityId, - id: location.id, - }, - }) - } + onClick={handleDelete} className={cn( buttonVariants({ variant: "destructive" }), )} >Don't forget to add the import:
-import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { useCallback } from "react";
1-5: Order imports alphabetically.For better code organization, consider ordering imports alphabetically within their groups.
import { useMutation, useQueryClient } from "@tanstack/react-query"; import { ChevronRight, Folder, FolderOpen, PenLine } from "lucide-react"; import { Link } from "raviger"; +import { useCallback } from "react"; import { useTranslation } from "react-i18next"; import { toast } from "sonner";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/settings/locations/components/LocationCard.tsx (1)
120-162: Good work on implementing deletion restrictions.You've correctly implemented the business logic to prevent deletion of locations that:
- Have child locations (
!location.has_children)- Are currently in use (
!location.current_encounter)This aligns with the requirements discussed in the PR comments and ensures data integrity.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Screenshot
Merge Checklist
Summary by CodeRabbit