Fix : Button Refreshes Entire Organizations #10978
Conversation
WalkthroughThe changes introduce a new state variable, Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. |
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => removeOrganization(org.id)} | ||
| disabled={isRemoving} | ||
| disabled={removingOrgId === org.id} | ||
| > | ||
| {isRemoving ? ( | ||
| {removingOrgId === org.id ? ( | ||
| <Loader2 className="h-4 w-4 animate-spin" /> | ||
| ) : ( | ||
| <Trash2 className="h-4 w-4 text-destructive" /> |
There was a problem hiding this comment.
i'd say wrap this button in a component and keep the useMutation there in that component, and use useMutation's isPending to show the loading state instead of creating a new state here;
![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
🧹 Nitpick comments (3)
src/components/Patient/LinkDepartmentsSheet.tsx (3)
100-131: Consider extracting the error handling logic to reduce duplication.The error handling logic is duplicated between this mutation and the
addOrganizationmutation. This could be extracted into a reusable utility function to follow the DRY principle.// Add a utility function at the top of the file +function handleMutationError(error: any) { + const errorData = error.cause as { errors: { msg: string }[] }; + errorData.errors.forEach((er) => { + toast.error(er.msg); + }); +} // Then in the DeleteOrganizationButton component onError: (error) => { - const errorData = error.cause as { errors: { msg: string }[] }; - errorData.errors.forEach((er) => { - toast.error(er.msg); - }); + handleMutationError(error); },
133-146: Add accessibility attributes to the button.Since the button only contains an icon, it should have an appropriate
aria-labelfor screen readers.<Button variant="ghost" size="icon" onClick={() => removeOrganization(organizationId)} disabled={isPending} + aria-label={t("remove_organization")} > {isPending ? ( <Loader2 className="h-4 w-4 animate-spin" /> ) : ( <Trash2 className="h-4 w-4 text-destructive" /> )} </Button>
174-180: Refactor query invalidation for consistency.The
addOrganizationmutation usesgetInvalidateQueries, but theDeleteOrganizationButtonusesgetMutationParams(...).queryKey. For consistency, consider using the same approach for both.onSuccess: () => { - const invalidateQueries = getInvalidateQueries(entityType, entityId); - queryClient.invalidateQueries({ queryKey: invalidateQueries }); + const { queryKey } = getMutationParams( + entityType, + entityId, + facilityId, + true, + ); + queryClient.invalidateQueries({ queryKey }); toast.success("Organization added successfully"); setSelectedOrg(""); setOpen(false); onUpdate?.(); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/LinkDepartmentsSheet.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Patient/LinkDepartmentsSheet.tsx (2)
87-147: Well-implemented component extraction!This is a great implementation of the component extraction approach. By moving the delete functionality into its own component:
- The loading state is now properly managed with
isPendingfromuseMutation- The component is reusable and has a clear, focused responsibility
- The UI feedback during deletion operations is improved with the loading spinner
This directly addresses the issue mentioned in the PR title about refreshing entire organizations, as the loading state is now localized to just the button being clicked.
245-251: Good implementation of component reuse!Excellent use of the extracted component with all the necessary props passed correctly. The inclusion of the
onSuccesscallback to trigger theonUpdatefunction ensures that any parent components stay in sync with the changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/LinkDepartmentsSheet.tsx (1)
92-119: Consider consistent loading state pattern across mutations.While the
removeOrganizationmutation now uses organization-specific loading states, theaddOrganizationmutation still uses a globalisAddingstate. For consistency, you might want to apply the same pattern to both mutations.- const { mutate: addOrganization, isPending: isAdding } = useMutation({ + const [addingOrg, setAddingOrg] = useState<boolean>(false); + const { mutate: addOrganization } = useMutation({ mutationFn: (organizationId: string) => { + setAddingOrg(true); const { route, pathParams } = getMutationParams( entityType, entityId, facilityId, true, ); // rest of the code... }, onSuccess: () => { // existing code... + setAddingOrg(false); }, onError: (error) => { + setAddingOrg(false); // existing error handling... }, });Then update the button:
- disabled={!selectedOrg || isAdding} + disabled={!selectedOrg || addingOrg} > - {isAdding && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} + {addingOrg && <Loader2 className="mr-2 h-4 w-4 animate-spin" />}Also applies to: 120-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/LinkDepartmentsSheet.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/components/Patient/LinkDepartmentsSheet.tsx (6)
90-90: Good addition of organization-specific loading state tracking.This state variable provides granular control over loading indicators for each organization's removal button, a better approach than the previous global loading state.
120-120: Correctly removed the global isPending state tracking.The change aligns with the new approach of tracking loading states at the individual organization level rather than globally.
122-122: Good implementation of loading state management.Setting the
removingOrgIdat the start of the mutation properly identifies which organization is currently being processed.
143-143: Proper cleanup of loading state in both success and error paths.The code correctly resets the
removingOrgIdstate regardless of whether the operation succeeds or fails, ensuring the UI won't get stuck in a loading state.Also applies to: 147-147
214-214: Improved UI feedback during organization removal.The button now correctly:
- Disables only the specific organization being removed
- Shows a loading spinner only for the specific organization being removed
- Prevents multiple removal operations from being initiated simultaneously
This change directly addresses the issue described in the PR title by preventing the entire organization list from refreshing during removal operations.
Also applies to: 216-220
211-219: Consider alternative implementation approach for better maintainability.In response to a previous review comment about wrapping the button in a component, I'd like to note that this is still a valid suggestion worth considering for future refactoring. A dedicated
RemoveOrganizationButtoncomponent would improve:
- Code organization by encapsulating the removal functionality
- Reusability if this pattern is used elsewhere
- Testing by isolating the functionality
A component-based implementation might look like this:
// RemoveOrganizationButton.tsx function RemoveOrganizationButton({ organizationId, onRemove, onSuccess, onError }) { const [isRemoving, setIsRemoving] = useState(false); const handleRemove = async () => { setIsRemoving(true); try { await onRemove(organizationId); onSuccess?.(); } catch (error) { onError?.(error); } finally { setIsRemoving(false); } }; return ( <Button variant="ghost" size="icon" onClick={handleRemove} disabled={isRemoving} > {isRemoving ? ( <Loader2 className="h-4 w-4 animate-spin" /> ) : ( <Trash2 className="h-4 w-4 text-destructive" /> )} </Button> ); }Then use it in the main component:
<RemoveOrganizationButton organizationId={org.id} onRemove={removeOrganization} onSuccess={() => queryClient.invalidateQueries({ queryKey })} onError={(error) => { const errorData = error.cause as { errors: { msg: string }[] }; errorData.errors.forEach((er) => { toast.error(er.msg); }); }} />
Proposed Changes
fix34.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit