Fixed : Delete Button in Schedules #9739
Conversation
WalkthroughThe pull request introduces state management improvements and internationalization updates in two components: 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: 2
🧹 Nitpick comments (3)
src/components/Users/UserAvailabilityTab.tsx (1)
53-53: Dependency array might cause unnecessary calls
Includingtin the dependency array causes re-rendering if the translation function changes (e.g., language switch). This is often desired for up-to-date translations, but ensure it doesn't trigger undesired extra calls.src/components/Schedule/ScheduleTemplatesList.tsx (2)
47-90: Data fetching approach
Consider adding retry logic or user prompts (e.g., “Retry” button) in case of transient network failures.
130-135: Internationalization consistency
For better maintainability, consider using a key-based translation for “No schedule templates found for this month.” rather than passing the full sentence tot().
📜 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 (2)
src/components/Schedule/ScheduleTemplatesList.tsx(6 hunks)src/components/Users/UserAvailabilityTab.tsx(5 hunks)
🔇 Additional comments (15)
src/components/Users/UserAvailabilityTab.tsx (5)
3-3: Proper import for i18n
The addition of theuseTranslationimport is well-structured and consistent with React-i18next usage.
45-45: Standard usage of translation hook
Using destructuring to retrieve thetfunction fromuseTranslationis clear and idiomatic.
164-164: Internationalizing “Add Exception”
Using thetfunction helps maintain consistency with your localization efforts.
230-230: Internationalizing “Schedule”
Good adoption oftto ensure the text is translated.
240-240: Internationalizing “Exceptions”
This change aligns with the app's multilingual goals by removing hard-coded strings.src/components/Schedule/ScheduleTemplatesList.tsx (10)
3-3: No issues with the new import
Your import ofuseStateanduseEffectis necessary for the new stateful logic.
18-18: Import ofScheduleAPIslooks good
This import is properly set up to handle schedule-related API endpoints.
29-30: Hooks usage
LeveraginguseSluganduseToastfor retrieving the facility ID and managing toast notifications is a clean approach.
39-42: State initialization
InitializingscheduleTemplateswithitemsis concise and ensures existing data is readily available without extra fetches.
141-143: Clean list rendering
Mapping overscheduleTemplatesis straightforward and well-structured.
150-152: Interface extension
ScheduleTemplateItemPropsextendsScheduleTemplateand adds anonDeletecallback, which keeps the component interface cohesive.
Line range hint
154-190: Verify slot_type range
Check that(slot.slot_type as unknown as number)is reliably ≥ 1. If it can be 0 or negative, you might get an out-of-bounds error.
219-219: Use of “slots_of”
No issues with the translation key usage here; it follows established patterns.
232-235: Date formatting
Well done usingformatfrom date-fns in tandem with the translation placeholder.
32-33: Request library usage
Ensure that therequestutility provides robust error handling for timeouts and network failures.Here’s a shell script to check for consistent error handling in all
requestcalls:✅ Verification successful
Let me check the request utility implementation to verify error handling.
Let me try a different approach to find the request utility implementation.
Let me check for the error handling implementation.
Based on the examination of the request utility implementation and error handling, I can now provide the final response:
The request utility has robust error handling in place
The implementation includes comprehensive error handling for:
- Network failures and timeouts (through native fetch API error handling)
- HTTP error status codes (400-599)
- Authentication failures with session expiry handling
- Bad requests with detailed error messages
- Content type handling for different response types
- Silent mode option for controlled error notifications
The review comment's concern about error handling is addressed by the existing implementation in:
- src/Utils/request/request.ts (main request utility)
- src/Utils/request/handleResponse.ts (centralized error handling)
The implementation properly catches errors, provides appropriate notifications, and handles edge cases like authentication failures gracefully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for request calls to confirm consistent error handling across the repo rg -A 5 'request\('Length of output: 19206
Script:
#!/bin/bash # Check the request utility implementation ast-grep --pattern 'export $_ function request' -A 30Length of output: 55
Script:
#!/bin/bash # Find and examine the request utility file fd request.ts --type f --exec cat {}Length of output: 7015
Script:
#!/bin/bash # Find and examine the handleResponse utility fd handleResponse.ts --type f --exec cat {}Length of output: 1183
| useEffect(() => { | ||
| if (!facilityId) { | ||
| toast.error("User needs to be linked to a home facility"); | ||
| toast.error(t("user_needs_to_be_linked_to_home_facility")); |
There was a problem hiding this comment.
💡 Codebase verification
Missing translation key detected
The translation key user_needs_to_be_linked_to_home_facility is not found in any of the locale files (en, hi, kn, ml, mr, ta) under public/locale/, but it is being used in src/components/Users/UserAvailabilityTab.tsx. This will likely result in missing text or fallback issues in the UI.
- Add the translation key and its corresponding text to all locale files under
public/locale/ - Start with
public/locale/en.jsonfor the English text, then ensure translations are added to other language files
🔗 Analysis chain
Ensure translation key is defined
It's best practice to verify that the key "user_needs_to_be_linked_to_home_facility" is present in your translation resources to avoid missing-text fallback issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation key is present in the locale files
rg "user_needs_to_be_linked_to_home_facility"
Length of output: 156
Script:
#!/bin/bash
# Search for translation files in common locations
fd -e json -e yaml -e yml translations
fd -e json -e yaml -e yml locales
fd -e json -e yaml -e yml i18n
# Let's also check the content of these files if found
fd -e json -e yaml -e yml "translations|locales|i18n" --exec cat {}
Length of output: 181
Script:
#!/bin/bash
# Let's try a broader search for translation files and keys
# First, let's find all json/yaml files that might contain translations
fd -e json -e yaml -e yml .
# Also search for any file that might contain this specific translation key
rg -l "user_needs_to_be_linked_to_home_facility" --type json --type yaml --type typescript --type javascript
Length of output: 773
Script:
#!/bin/bash
# Let's check the content of locale files to find the translation key
cat public/locale/en.json | grep -i "user_needs_to_be_linked_to_home_facility"
cat public/locale/hi.json | grep -i "user_needs_to_be_linked_to_home_facility"
cat public/locale/kn.json | grep -i "user_needs_to_be_linked_to_home_facility"
cat public/locale/ml.json | grep -i "user_needs_to_be_linked_to_home_facility"
cat public/locale/mr.json | grep -i "user_needs_to_be_linked_to_home_facility"
cat public/locale/ta.json | grep -i "user_needs_to_be_linked_to_home_facility"
Length of output: 473
| const handleDelete = async (id: string) => { | ||
| if (!scheduleTemplates || !facilityId) return; | ||
|
|
||
| try { | ||
| const { error } = await request(ScheduleAPIs.templates.delete, { | ||
| pathParams: { facility_id: facilityId, id }, | ||
| }); | ||
|
|
||
| if (error) { | ||
| throw new Error(t("failed_to_delete_template")); | ||
| } | ||
|
|
||
| setScheduleTemplates((prevTemplates) => | ||
| prevTemplates | ||
| ? prevTemplates.filter((template) => template.id !== id) | ||
| : [], | ||
| ); | ||
|
|
||
| toast({ | ||
| title: t("success"), | ||
| description: t("schedule_template_deleted_successfully"), | ||
| variant: "default", | ||
| }); | ||
| } catch (error) { | ||
| console.error(t("failed_to_delete_schedule_template"), error); | ||
| toast({ | ||
| title: t("error"), | ||
| description: t( | ||
| "an_error_occurred_while_deleting_the_schedule_template", | ||
| ), | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deletion confirmation
Ensure users can’t accidentally delete a schedule template by adding a confirmation dialog.
Here’s a possible approach:
+const handleDelete = async (id: string) => {
+ if (!window.confirm("Are you sure you want to delete this item?")) {
+ return;
+ }
+ // Existing code
+};Committable suggestion skipped: line range outside the PR's diff.
|
most of the changes present here seems to be unrelated to solve the issue. closing it |
|
@rithviknishad, Used useMutation. Do I need to create a new PR, or can you reopen this PR again? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Schedule/ScheduleTemplatesList.tsx (4)
4-4: ImportingReact
Since React 17+, importingReactis optional for JSX transforms, but having it is not harmful and can improve clarity.
49-73: Well-handled delete mutation
Using a React Query mutation with onSuccess and onError callbacks is an excellent pattern for consistent feedback to users. Consider adding a success toast for user confirmation as well.
93-93: Internationalization string handling
Directly passing a sentence string totis acceptable, though some projects prefer using unique keys (e.g.,t("no_schedule_templates_found_for_this_month")). This is a minor nitpick and depends on your i18n conventions.
100-102: Prop spreading inScheduleTemplateItem
Using the spread operator ({...template}) is concise. Consider explicitly passing only the needed props to make the component API clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Schedule/ScheduleTemplatesList.tsx(7 hunks)
🔇 Additional comments (12)
src/components/Schedule/ScheduleTemplatesList.tsx (12)
2-2: Good usage of@tanstack/react-queryimports
LeveraginguseMutationanduseQueryClientpromotes a clean approach to data updates throughout your application.
19-19: Consistent use ofScheduleAPIs
Pulling in theScheduleAPIsaligns with the rest of the codebase. Looks good.
30-34: Usage of custom hooks
Good job integratinguseSlug,useToast, andrequestfrom your utility modules. Ensure these hooks and utilities are well-tested to avoid edge cases.
40-48: Proper initialization of local state
InitializingscheduleTemplatesandisLoadingbased on the incomingitemsprop is logical. This approach ensures your component handles both loaded and pending states appropriately.
74-77: Revisit confirmation flow before deletion
Adding a prompt to confirm deletion would help prevent accidental removals of schedule templates.
78-83: Check for potential re-render conditions
TheuseEffectupdates the state wheneveritemschanges. This is a good pattern to ensure consistency, but verify that subsequent re-renders won't override any unsaved local modifications toscheduleTemplates.
85-89: Clear loading state management
Conditionally rendering<Loading />is a straightforward way to handle asynchronous data. The logic is clean and easy to read.
109-111: Well-defined interface
ExtendingScheduleTemplatewithonDeleteis simple and integrates nicely with the existing data structure.
113-116: ExtractingonDeleteand applying translation
DestructuringonDeleteand addingtensures a straightforward and properly localized component. This is a clean implementation.
139-149: Consider user confirmation for destructive action
Right now, the "Delete" action is triggered immediately on this menu item click. A confirmation step here would reduce user mistakes.
178-178: Time slot labeling
Your use oft("slots_of")clarifies the label for translated audiences. Ensure related time units are consistently localized (e.g., "mins").
191-195: Consistent date range display
Wrapping date outputs instrongis a nice way to highlight key information. The translation usage for "valid_from" and "till" is straightforward and effective.
There was a problem hiding this comment.
Most of the changes are still irrelavant. Also DO NOT use request within useQuery. request does not throw for HTTP Errors, but useQuery needs such things to be thrown in order to handle toast notifications accordingly.
Refer: https://docs.ohc.network/docs/care/development/data-fetching-in-care/
Also PLEASE DISCARD LOCKFILE CHANGES WHEN MAKING PR UNLESS NECESSARY. I guess this has been flagged in most of your PRs by now.
|
@rithviknishad, I have made changes like removing request and removed notification for deletion(toast), Could you please check it out by re-opening the PR, if there are any other unnecessary codes. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Schedule/ScheduleTemplatesList.tsx (1)
47-66:⚠️ Potential issueAdd error handling and user confirmation for delete operation
The delete mutation implementation is good, but there are two important improvements needed:
- Add error handling to show feedback when deletion fails
- Add confirmation dialog before deletion (as previously suggested)
Here's how to improve the implementation:
const deleteMutation = useMutation({ mutationFn: (id: string) => mutate(ScheduleAPIs.templates.delete, { pathParams: { facility_id: facilityId!, id }, })({}), onSuccess: (_, variables) => { setScheduleTemplates((prevTemplates) => prevTemplates ? prevTemplates.filter((template) => template.id !== variables) : [], ); queryClient.invalidateQueries({ queryKey: ["scheduleTemplates", facilityId], }); }, + onError: (error) => { + // Assuming you have a toast notification system + toast.error(t("Failed to delete schedule template")); + }, }); const handleDelete = (id: string) => { if (!scheduleTemplates || !facilityId) return; + if (!window.confirm(t("Are you sure you want to delete this template?"))) { + return; + } deleteMutation.mutate(id); };
🧹 Nitpick comments (3)
src/components/Schedule/ScheduleTemplatesList.tsx (3)
39-72: Consider adding cleanup for state updatesWhile the state management is well-implemented, there's a potential race condition in the useEffect hook. If the component unmounts while items are being loaded, you might have state updates on an unmounted component.
Consider adding a cleanup function:
useEffect(() => { + let mounted = true; if (items !== undefined) { - setScheduleTemplates(items); - setIsLoading(false); + if (mounted) { + setScheduleTemplates(items); + setIsLoading(false); + } } + return () => { + mounted = false; + }; }, [items]);
Line range hint
98-138: Show loading and error states during deletionThe component interface and i18n implementation look good, but the UI should reflect the mutation state during deletion operations.
Consider updating the delete button to show loading state:
<DropdownMenuItem className="cursor-pointer" + disabled={deleteMutation.isPending} onClick={() => onDelete(props.id)} > - {t("delete")} + {deleteMutation.isPending ? t("deleting...") : t("delete")} </DropdownMenuItem>
Line range hint
1-191: Overall implementation feedbackThe implementation successfully addresses the delete functionality with good practices for state management, TypeScript usage, and i18n support. However, before merging, please address the following critical items:
- Add confirmation dialog for delete operations
- Implement error handling with user feedback
- Show loading states during deletion
- Add cleanup in useEffect to prevent memory leaks
The changes align well with the PR objectives and maintain good code quality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Schedule/ScheduleTemplatesList.tsx(7 hunks)
🔇 Additional comments (1)
src/components/Schedule/ScheduleTemplatesList.tsx (1)
2-4: Well-structured imports!The added imports are appropriate for implementing the delete functionality with proper state management and internationalization support.
Also applies to: 19-19, 30-32
Taking it up myself, as I've done this in my upcoming PR. |
Proposed Changes
Delete_button.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor