New List view for locations in the facility settings #10398
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a new localization entry and significant updates to the facility location settings. The JSON file now includes a new key for filtering by location. The 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: 3
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (1)
42-48: Consider adding recursion depth limit.The recursive rendering of LocationChildren could potentially cause performance issues with deeply nested locations.
Add a maximum depth limit:
{childLocation.has_children && ( + level < 5 && ( <LocationChildren facilityId={facilityId} location={childLocation} level={level + 1} /> + ) )}src/pages/Facility/settings/locations/LocationList.tsx (2)
75-94: Add aria-label for better accessibility.The tabs component lacks proper accessibility attributes.
Add aria labels:
<Tabs value={activeTab} onValueChange={(value) => setActiveTab(value as "card" | "list")} className="ml-auto" + aria-label="View options" > <TabsList className="flex"> <TabsTrigger value="card" id="user-card-view">
135-141: Add sorting functionality to table headers.The table headers could benefit from sorting functionality for better data organization.
Consider adding sortable columns:
<TableHeader> <TableRow className="divide-x bg-gray-100"> - <TableHead>{t("location")}</TableHead> - <TableHead>{t("location_form")}</TableHead> + <TableHead> + <button onClick={() => handleSort('location')} className="flex items-center"> + {t("location")} + <CareIcon icon={sortOrder === 'asc' ? 'l-arrow-up' : 'l-arrow-down'} /> + </button> + </TableHead> + <TableHead> + <button onClick={() => handleSort('form')} className="flex items-center"> + {t("location_form")} + <CareIcon icon={sortOrder === 'asc' ? 'l-arrow-up' : 'l-arrow-down'} /> + </button> + </TableHead> </TableRow> </TableHeader>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/pages/Facility/settings/locations/LocationList.tsx(3 hunks)src/pages/Facility/settings/locations/components/LocationChildren.tsx(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/pages/Facility/settings/locations/components/LocationChildren.tsx (1)
15-19: LGTM! Props interface is well-defined.The Props interface clearly defines the required types for the component parameters.
public/locale/en.json (1)
1016-1016: LGTM! New localization entry is consistent.The new entry "filter_location" follows the existing naming convention and format.
| const { data: children } = useQuery({ | ||
| queryKey: ["locations", facilityId, location?.id, "children"], | ||
| queryFn: query.debounced(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| parent: location?.id, | ||
| }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Add error handling for the query.
The query implementation lacks error handling which could lead to runtime issues if the API call fails.
Add error state handling:
- const { data: children } = useQuery({
+ const { data: children, error } = useQuery({
queryKey: ["locations", facilityId, location?.id, "children"],
queryFn: query.debounced(locationApi.list, {
pathParams: { facility_id: facilityId },
queryParams: {
parent: location?.id,
},
}),
});
+
+ if (error) {
+ return <div>Error loading child locations: {error.message}</div>;
+ }📝 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 { data: children } = useQuery({ | |
| queryKey: ["locations", facilityId, location?.id, "children"], | |
| queryFn: query.debounced(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: location?.id, | |
| }, | |
| }), | |
| }); | |
| const { data: children, error } = useQuery({ | |
| queryKey: ["locations", facilityId, location?.id, "children"], | |
| queryFn: query.debounced(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: location?.id, | |
| }, | |
| }), | |
| }); | |
| if (error) { | |
| return <div>Error loading child locations: {error.message}</div>; | |
| } |
| return children?.results?.map((childLocation: LocationListType) => ( | ||
| <React.Fragment key={childLocation.id}> | ||
| <TableRow className="divide-x font-medium cursor-pointer bg-gray-50 dark:bg-gray-800"> | ||
| <TableCell className={paddingClasses[level]}> | ||
| {" "} | ||
| {/* Indentation for hierarchy */} | ||
| <CareIcon icon="l-corner-down-right" /> {childLocation.name} | ||
| </TableCell> | ||
| <TableCell>{getLocationFormLabel(childLocation?.form)}</TableCell> | ||
| </TableRow> | ||
| {childLocation.has_children && ( | ||
| <LocationChildren | ||
| facilityId={facilityId} | ||
| location={childLocation} | ||
| level={level + 1} | ||
| /> | ||
| )} | ||
| </React.Fragment> | ||
| )); |
There was a problem hiding this comment.
Add null check and loading state for children data.
The component doesn't handle the loading state or null children data, which could cause rendering issues.
Add proper checks:
+ if (!children?.results) {
+ return null;
+ }
return children?.results?.map((childLocation: LocationListType) => (
<React.Fragment key={childLocation.id}>📝 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.
| return children?.results?.map((childLocation: LocationListType) => ( | |
| <React.Fragment key={childLocation.id}> | |
| <TableRow className="divide-x font-medium cursor-pointer bg-gray-50 dark:bg-gray-800"> | |
| <TableCell className={paddingClasses[level]}> | |
| {" "} | |
| {/* Indentation for hierarchy */} | |
| <CareIcon icon="l-corner-down-right" /> {childLocation.name} | |
| </TableCell> | |
| <TableCell>{getLocationFormLabel(childLocation?.form)}</TableCell> | |
| </TableRow> | |
| {childLocation.has_children && ( | |
| <LocationChildren | |
| facilityId={facilityId} | |
| location={childLocation} | |
| level={level + 1} | |
| /> | |
| )} | |
| </React.Fragment> | |
| )); | |
| if (!children?.results) { | |
| return null; | |
| } | |
| return children?.results?.map((childLocation: LocationListType) => ( | |
| <React.Fragment key={childLocation.id}> | |
| <TableRow className="divide-x font-medium cursor-pointer bg-gray-50 dark:bg-gray-800"> | |
| <TableCell className={paddingClasses[level]}> | |
| {" "} | |
| {/* Indentation for hierarchy */} | |
| <CareIcon icon="l-corner-down-right" /> {childLocation.name} | |
| </TableCell> | |
| <TableCell>{getLocationFormLabel(childLocation?.form)}</TableCell> | |
| </TableRow> | |
| {childLocation.has_children && ( | |
| <LocationChildren | |
| facilityId={facilityId} | |
| location={childLocation} | |
| level={level + 1} | |
| /> | |
| )} | |
| </React.Fragment> | |
| )); |
| } | ||
|
|
||
| export function LocationChildren({ facilityId, location, level }: Props) { | ||
| const paddingClasses = ["pl-4", "pl-8", "pl-12", "pl-16", "pl-20"]; |
There was a problem hiding this comment.
this is not required if we are doing recursive rendering right? what if there are more levels?
There was a problem hiding this comment.
Hmm, with queries being tied to the stack itself, it doesn't look good (when it's rendered live like that).
We can call /facility/facilityId/location/ to get all the associated locations. You can then use parent Id to collate the data into an object, and then render component based on that object.
I'd suggest adding two behaviors:
(1) By default when clicking on an root, only render its own children, rather than everything.
(2) Allow user to click on each item to render its children
(3) Add a show all button to the root to show the entire structure under root.
You can check #10411 for similar behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (1)
27-35:⚠️ Potential issueAdd error handling for the query.
The query implementation lacks error handling which could lead to runtime issues if the API call fails.
Apply this diff to add error handling:
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, error } = useQuery({ queryKey: ["locations", facilityId, location?.id, "children"], queryFn: query.debounced(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location?.id, }, }), }); + if (error) { + return <div className="text-red-500 p-4">Error loading child locations</div>; + }src/pages/Facility/settings/locations/LocationList.tsx (1)
91-100: 🛠️ Refactor suggestionAdd debouncing to search input.
The search input updates on every keystroke which could lead to unnecessary API calls.
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (1)
71-72: Improve loading state presentation.The current loading state implementation could be enhanced for better user experience.
Apply this diff:
- {childLocation.name} - {isLoading ? <span>Loading...</span> : null} + <div className="flex items-center gap-2"> + <span>{childLocation.name}</span> + {isLoading && ( + <span className="inline-flex items-center px-2 py-1 text-xs bg-gray-100 rounded-full"> + Loading... + </span> + )} + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/pages/Facility/settings/locations/LocationList.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationChildren.tsx(1 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 (4)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (2)
16-20: LGTM! Props interface is well-defined.The Props interface clearly defines the required properties with appropriate types.
37-47: LGTM! Clean implementation of toggle functionality.The toggleExpand function efficiently manages state using Set, which is perfect for this use case.
src/pages/Facility/settings/locations/components/LocationChildren.tsx
Outdated
Show resolved
Hide resolved
| <Button | ||
| variant="outline" | ||
| className="mr-3" | ||
| onClick={() => setSelectedLocation(null)} | ||
| > | ||
| <CareIcon icon="l-plus" className="w-4 h-5" /> | ||
| Expand All | ||
| </Button> |
There was a problem hiding this comment.
Fix "Expand All" button functionality.
The current implementation sets selectedLocation to null, which contradicts the "Expand All" label. This button should expand all locations instead of collapsing them.
Apply this diff:
- onClick={() => setSelectedLocation(null)}
+ onClick={(e) => {
+ e.stopPropagation();
+ const allLocationIds = data?.results.map(loc => loc.id) || [];
+ setExpandedLocations(new Set(allLocationIds));
+ }}Note: You'll need to lift the expandedLocations state up to the LocationList component and pass it down to LocationChildren.
📝 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.
| <Button | |
| variant="outline" | |
| className="mr-3" | |
| onClick={() => setSelectedLocation(null)} | |
| > | |
| <CareIcon icon="l-plus" className="w-4 h-5" /> | |
| Expand All | |
| </Button> | |
| <Button | |
| variant="outline" | |
| className="mr-3" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| const allLocationIds = data?.results.map((loc) => loc.id) || []; | |
| setExpandedLocations(new Set(allLocationIds)); | |
| }} | |
| > | |
| <CareIcon icon="l-plus" className="w-4 h-5" /> | |
| Expand All | |
| </Button> |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (2)
33-41:⚠️ Potential issueAdd error handling for the query.
The query implementation lacks error handling which could lead to runtime issues if the API call fails.
Apply this diff:
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, error } = useQuery({ queryKey: ["locations", facilityId, location?.id, "children"], queryFn: query.debounced(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location?.id, }, }), }); + + if (error) { + return <div>Error loading child locations: {error.message}</div>; + }
73-73:⚠️ Potential issueFix dynamic class name generation for Tailwind.
Using template literals for class names can cause issues with Tailwind's purge feature.
Apply this diff:
- <TableCell className={`flex items-center pl-${level * 4}`}> + <TableCell className={`${getPaddingClass(level)} flex items-center`}>Add this helper function at the top of the file:
const getPaddingClass = (level: number) => { const classes = ['pl-4', 'pl-8', 'pl-12', 'pl-16', 'pl-20']; return classes[Math.min(level, classes.length - 1)]; };src/pages/Facility/settings/locations/LocationList.tsx (2)
96-104: 🛠️ Refactor suggestionAdd debouncing to search input.
The search input updates on every keystroke which could lead to unnecessary API calls.
Implement debouncing:
+import { useDebouncedCallback } from 'use-debounce'; +const debouncedSearch = useDebouncedCallback((value) => { + setSearchQuery(value); +}, 300); <Input placeholder={t("filter_location")} value={searchQuery} onChange={(e) => { - setSearchQuery(e.target.value); + debouncedSearch(e.target.value); }} className="w-full" />
140-148: 🛠️ Refactor suggestionEnhance keyboard accessibility for interactive elements.
The table rows need proper keyboard interaction support and ARIA attributes.
Apply this diff:
<TableRow onClick={(e) => { e.stopPropagation(); setSelectedLocation( selectedLocation?.id === location.id ? null : location, ); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setSelectedLocation( + selectedLocation?.id === location.id ? null : location, + ); + } + }} + role="button" + tabIndex={0} className="divide-x font-medium cursor-pointer bg-white dark:bg-gray-900" >
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (1)
88-88: Enhance loading state visualization.The current loading state is a simple text. Consider using a proper loading spinner for better UX.
Apply this diff:
- {isLoading ? <span>Loading...</span> : null} + {isLoading ? ( + <div className="ml-2 animate-spin"> + <CareIcon icon="l-spinner" className="w-4 h-4" /> + </div> + ) : null}
📜 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 (3)
public/locale/en.json(4 hunks)src/pages/Facility/settings/locations/LocationList.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationChildren.tsx(1 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 (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/locations/components/LocationChildren.tsx (2)
1-21: LGTM! Well-organized imports and clear Props interface.The imports are logically grouped and the Props interface is well-typed with clear property names.
29-63: LGTM! Efficient state management implementation.The use of Set for expandedLocations and the implementation of toggleExpand and useEffect are well done.
src/pages/Facility/settings/locations/LocationList.tsx (1)
65-67: LGTM! "Expand All" button implementation is correct.The button correctly toggles the expandedAll state and the UI reflects the current state.
Also applies to: 164-171
|
@Jacobjeevan made the requested changes.can you review the changes |
| facilityId={facilityId} | ||
| location={childLocation} | ||
| level={level + 1} | ||
| expandedAll={expandedAll} |
There was a problem hiding this comment.
(1) Note what happens to other buttons when I click expand all on one.
(2) If I expand one level (and it still has other sub-levels to expand), clicking on expand all should continue expanding, not collapsing the results.
(3) Read my previous review. You still have a query attached to each child.
(4) Clicking on expand all - results should be instantaneous, user shouldn't have to wait. This is probably related to the query being attached to each level.
Proposed Changes
Fixes Add a new list view to locations in the facility settings #10376
Change 1
Change 2
More?
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit