Fix: Add a new list view to locations in the facility settings #10672
Conversation
WalkthroughThis pull request enhances the application by adding new localization keys in the English locale file, modifying the facilities locations list component, and introducing a new view component for rendering location details. The localization updates improve clarity for UI interactions, while the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LL as LocationList Component
participant LV as LocationListView Component
participant LR as LocationRow Component
U->>LL: Navigate to Locations Page
LL->>LL: Build location hierarchy & update state (activeTab, expandedRows)
alt List View Selected
LL->>LV: Render locations in table view
U->>LV: Search/filter locations
U->>LR: Click to expand/collapse a row
else Map View Selected
LL->>Map: Render map view with locations
end
Possibly related PRs
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/pages/Facility/settings/locations/LocationList.tsx (5)
48-48: Consider pagination or infinite scroll for large data sets.
Using a limit of 1000 could cause performance issues if the result set grows.
55-55: Double-check the large limit usage.
Filtering on the front end for up to 1000 locations can work, but dynamic pagination is typically more scalable.
79-81: Toggle row logic is straightforward.
However, for very large lists, you might memoize or optimize rerenders.
93-238:LocationRowis large—consider extracting reusable parts.
This chunk handles row expansion, nested children, and top-level logic. While functional, the code can be split into smaller, more focused components or hooks for clarity and maintainability.
294-409: Main return block is quite lengthy.
Moving the tab-based UI, info alerts, and search form to smaller sub-components can enhance readability. Otherwise, the logic is consistent and user-friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(7 hunks)src/pages/Facility/settings/locations/LocationList.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (23)
src/pages/Facility/settings/locations/LocationList.tsx (13)
2-3: No issues with these new imports.
These straightforward imports forPenLineandLinklook good.
12-19: Table components import looks fine.
Importing these UI table components is appropriate and helps maintain a clean, structured layout.
20-20: Tabs import confirmed.
ImportingTabsand related sub-components is correct for managing tabbed navigation.
22-22:Pagecomponent import is good.
Ensures consistent page layout styling throughout the app.
23-23: Skeleton loading import confirmed.
Allows a helpful loading placeholder while fetching data.
25-25:useBreakpointsimport approved.
Great custom hook approach for responsive design.
28-28:useViewimport looks good.
An effective way to persist tab view or route params.
30-30: Helper function import for location form label.
Neat utility for mapping form codes to friendly labels.
45-46: State for active tab and expanded rows.
Storing these as local states is straightforward. Just be mindful that a large data set could result in a largeexpandedRows.
51-52: Good use of query key.
IncludingfacilityIdandsearchQueryensures relevant caching behavior.
59-59: Conditional query enabling.
Usingenabled: !!facilityIdis a standard pattern to prevent unnecessary requests.
62-62: Data destructuring looks fine.
Falling back to an empty array is a useful safeguard.
240-292:renderListViewhandles empty/loading states well.
This is a clear approach that improves UX. Optionally, you can pre-filter data outside of the rendering function to keep rendering purely presentational.public/locale/en.json (10)
579-580: New translations for “click” and “click_on.”
These keys look appropriate for guiding user interactions.
583-583: “collapse_all” translation.
Concise and user-friendly for any UI expansions.
1000-1000: “expand_all” translation.
Clear labeling for expanding nested items.
1084-1084: “filter_by_locations” translation.
Useful for user filtering in the updated location list.
1146-1146: “hover_focus_reveal” translation.
Great for tooltips or subtle UI prompts.
1317-1317: “map” translation.
Essential for the upcoming map view.
1319-1319: “map_view_coming_soon” translation.
Accurately sets user expectation for future functionality.
1544-1544: “open_manage_sub_locations” translation.
Helps clarify the available action in the interface for nested location management.
2176-2177: Additional location-related strings.
“to_add_location” and “to_add_main_location” are helpful in guiding user steps.
2179-2179: “to_edit” translation.
A concise addition for clarifying edit-related actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/pages/Facility/settings/locations/LocationList.tsx (4)
39-56: Consider memoizing the hierarchy building function.The
buildLocationHierarchyfunction rebuilds the entire hierarchy on every render. While the current implementation is clean, it could benefit from memoization for larger datasets.-function buildLocationHierarchy(locations: LocationListType[]) { +const buildLocationHierarchy = (locations: LocationListType[]) => { const childrenMap = new Map<string, LocationListType[]>(); const topLevelLocations: LocationListType[] = []; // ... rest of the implementation return { childrenMap, topLevelLocations }; -} +}; + +const memoizedBuildLocationHierarchy = useMemo( + () => buildLocationHierarchy, + [] +);
145-161: Consider extracting the recursive toggle logic.The
toggleAllChildrenfunction contains complex recursive logic that could be moved outside the component for better maintainability and reusability.+const toggleChildrenRecursively = ( + getChildren: (parentId: string) => LocationListType[], + parentId: string, + expand: boolean, + newExpandedRows: Record<string, boolean> +) => { + getChildren(parentId).forEach((child) => { + newExpandedRows[child.id] = expand; + toggleChildrenRecursively(getChildren, child.id, expand, newExpandedRows); + }); +}; const toggleAllChildren = () => { setExpandedRows((prevExpandedRows) => { const newExpandedRows = { ...prevExpandedRows }; - const toggleChildren = (parentId: string, expand: boolean) => { - getChildren(parentId).forEach((child) => { - newExpandedRows[child.id] = expand; - toggleChildren(child.id, expand); - }); - }; const shouldExpand = !children.every( (child) => prevExpandedRows[child.id], ); newExpandedRows[location.id] = shouldExpand; - toggleChildren(location.id, shouldExpand); + toggleChildrenRecursively(getChildren, location.id, shouldExpand, newExpandedRows); return newExpandedRows; }); };
365-413: Consider extracting the info banner to a separate component.The info banner implementation is quite large and could be extracted into a reusable component, especially since it contains conditional rendering for desktop/mobile views.
+const LocationInfoBanner = () => { + const { t } = useTranslation(); + return ( + <div className="rounded-lg border-2 border-blue-200 bg-blue-50 p-4"> + {/* ... existing banner implementation ... */} + </div> + ); +}; {activeTab === "list" && ( - <div className="rounded-lg border-2 border-blue-200 bg-blue-50 p-4"> - {/* ... existing banner implementation ... */} - </div> + <LocationInfoBanner /> )}
415-421: Consider implementing a loading state for the map view.The map view placeholder could benefit from a loading state to maintain consistency with the list view.
{activeTab === "list" ? ( renderListView() ) : ( <div className="h-[600px] bg-gray-100 rounded-lg flex items-center justify-center"> + {isLoading ? ( + <div className="animate-pulse"> + <CareIcon icon="l-map" className="h-8 w-8 text-gray-400" /> + </div> + ) : ( <p className="text-gray-500">{t("map_view_coming_soon")}</p> + )} </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
87-104: LGTM! Efficient filtering implementation.The
getChildrenandfilteredTopLevelLocationsimplementations are well-optimized:
- Properly memoized to prevent unnecessary recalculations
- Case-insensitive search
- Clear separation of concerns
272-315: LGTM! Well-structured list view implementation.The
renderListViewfunction is well-organized with clear handling of:
- Loading states
- Empty states
- Search results
- Table layout
67-79:❓ Verification inconclusive
Verify the impact of the high LIMIT value.
Setting a constant
LIMITof 1000 might:
- Impact initial load performance
- Cause memory issues with large datasets
- Affect server response times
Consider implementing virtual scrolling for better performance with large datasets.
🏁 Script executed:
#!/bin/bash # Check for potential performance impact # Look for similar patterns in the codebase rg -A 2 "limit.*=.*1000" .Length of output: 28
Review update: Evaluate the Performance Impact of Using a High LIMIT Value
- The constant
LIMITof 1000 insrc/pages/Facility/settings/locations/LocationList.tsxmay lead to performance issues (e.g., increased memory usage, slower page loads, and extended server response times) if the dataset grows large.- Although a repository-wide search for similar patterns did not produce conclusive results, it's important to manually verify this behavior in typical deployment environments.
- Consider evaluating if the current data volume justifies the high limit. If not, exploring alternatives such as pagination or virtual scrolling could offer better performance and scalability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
39-56: Consider memoizing location hierarchy computation.The
buildLocationHierarchyfunction processes the entire location dataset on each render. While the implementation is correct, consider memoizing the results based on the locations array to prevent unnecessary recomputation.+ // Add useMemo to cache results + const memoizedHierarchy = useMemo(() => { const childrenMap = new Map<string, LocationListType[]>(); const topLevelLocations: LocationListType[] = []; locations.forEach((location) => { if (!location.parent || Object.keys(location.parent).length === 0) { topLevelLocations.push(location); } else { const parentId = location.parent.id; if (!childrenMap.has(parentId)) { childrenMap.set(parentId, []); } childrenMap.get(parentId)?.push(location); } }); return { childrenMap, topLevelLocations }; + }, [locations]);
125-270: Consider decomposing LocationRow for better maintainability.The
LocationRowcomponent handles multiple responsibilities including row rendering, expansion logic, and action buttons. Consider breaking it down into smaller, focused components:+ const LocationActions = ({ location, isMobile, handleEditLocation }: LocationActionsProps) => ( + <div className="flex items-center gap-2 ml-auto"> + <Button + variant="white" + size={isMobile ? "xs" : "sm"} + onClick={() => handleEditLocation(location)} + className="opacity-100 lg:opacity-0 group-hover:opacity-100 transition-opacity" + > + <PenLine className="h-4 w-4" /> + <span className="hidden lg:inline">{t("edit")}</span> + </Button> + {/* ... other actions */} + </div> + ); + const ExpandCollapseButton = ({ isExpanded, onClick, children }: ExpandCollapseProps) => ( + <Button + variant="white" + onClick={onClick} + className="opacity-100 lg:opacity-0 group-hover:opacity-100 transition-opacity gap-2" + > + {children} + </Button> + );This would improve:
- Code reusability
- Testing isolation
- Maintenance
272-315: Add error boundary protection for the list view.The
renderListViewfunction handles loading and empty states but lacks error handling. Consider wrapping it in an error boundary to gracefully handle rendering failures.+ class LocationListErrorBoundary extends React.Component { + state = { hasError: false }; + + static getDerivedStateFromError(error) { + return { hasError: true }; + } + + render() { + if (this.state.hasError) { + return ( + <Card className="col-span-full"> + <CardContent className="p-6 text-center text-gray-500"> + {t("error_loading_locations")} + </CardContent> + </Card> + ); + } + return this.props.children; + } + } const renderListView = () => { + return ( + <LocationListErrorBoundary> {/* existing render logic */} + </LocationListErrorBoundary> + ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(7 hunks)src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (2)
1-34: Well-organized imports and component setup!The imports are logically grouped and include all necessary UI components for the new table-based list view.
317-437: Well-implemented location list with comprehensive features!The implementation successfully:
- Replaces grid with a table layout
- Adds tab navigation between list and map views
- Provides clear user guidance through tooltips
- Implements responsive design for mobile/desktop
- Includes search functionality
public/locale/en.json (1)
579-580: Well-structured localization additions for the new location list view!The new localization keys:
- Follow consistent naming patterns
- Cover all new UI text requirements
- Support both desktop and mobile interfaces
Also applies to: 583-583, 1000-1000, 1084-1084, 2175-2178
|
Disable/commented out Map View as @shivankacker is working on that. (#10377) |
…r Styling Updates
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
67-67: 🛠️ Refactor suggestionConsider using pagination instead of a high limit.
Setting a high constant limit (
LIMIT = 1000) could impact performance with large datasets. Consider implementing proper pagination or infinite scroll.Based on the past review comment from Jacobjeevan suggesting to "switch to using query.paginated instead of high limit."
🧹 Nitpick comments (2)
src/pages/Facility/settings/locations/LocationList.tsx (2)
87-118: Optimize search implementation for better performance.The current search implementation recursively traverses the location hierarchy for each search operation. Consider:
- Implementing debounced search
- Memoizing intermediate search results
+import { useMemo, useCallback, useDebounce } from 'react'; + +const debouncedSearchQuery = useDebounce(searchQuery, 300); + +const searchCache = useMemo(() => new Map<string, boolean>(), [debouncedSearchQuery]); + const hasMatchingChildren = useCallback( (parentId: string): boolean => { + if (searchCache.has(parentId)) { + return searchCache.get(parentId)!; + } const children = childrenMap.get(parentId) || []; - return children.some( + const result = children.some( (child) => matchesSearch(child.name) || hasMatchingChildren(child.id), ); + searchCache.set(parentId, result); + return result; }, - [childrenMap, searchQuery], + [childrenMap, debouncedSearchQuery, searchCache], );
180-263: Improve accessibility of the location row component.Add ARIA attributes to enhance screen reader support:
<TableRow className="group hover:bg-muted/50" style={{ "--indent": `${indent}rem` } as React.CSSProperties} + role="row" + aria-expanded={isExpanded} > <TableCell className={`${ isTopLevel ? "bg-white font-bold text-gray-900" : "bg-white font-medium text-gray-900" } flex justify-between lg:flex-row flex-col pl-[var(--indent)] flex-wrap gap-2`} + role="cell" > <div className="flex items-center"> {isTopLevel || children.length > 0 ? ( <Button size="icon" variant="link" onClick={() => toggleRow(location.id)} + aria-label={`${isExpanded ? 'Collapse' : 'Expand'} ${location.name}`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Facility/settings/locations/LocationList.tsx (4)
39-56: LGTM! Efficient hierarchy building implementation.The
buildLocationHierarchyfunction efficiently organizes locations using a Map for O(n) time complexity.
374-417: LGTM! Clear and informative help text implementation.The info section provides clear instructions for users on both desktop and mobile views, with appropriate icons and responsive layout.
279-322: LGTM! Well-structured list view implementation.The
renderListViewfunction handles loading states, empty states, and data presentation effectively with clear separation of concerns.
342-349:❓ Verification inconclusive
Verify Map view implementation status.
The Map view is currently commented out with a note indicating it will be added later. This aligns with the PR objectives mentioning that @shivankacker is working on this feature in issue #10377.
🏁 Script executed:
#!/bin/bash # Check the status of the Map view implementation gh issue view 10377Length of output: 110
Action: Please manually verify the Map view implementation status
The commented-out Map view code appears to be intentionally left in place until @shivankacker’s work for issue #10377 is ready. However, the automated check using
gh issue view 10377failed due to the repository not being recognized as a git repository. As a result, we could not confirm the implementation status automatically. Please manually verify that issue #10377 is still open and that the Map view functionality is pending.
- Confirm that issue Add a new map view to the locations section of a facility settings #10377 correctly tracks the Map view feature.
- Ensure that leaving the map-related code commented out aligns with the current implementation plan.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/pages/Facility/settings/locations/LocationList.tsx (2)
84-114: Consider memoizing the search matcher function.The
matchesSearchfunction is recreated on every render. While it's a simple function, it's used in multiple places and could benefit from memoization.-const matchesSearch = (name: string) => - name.toLowerCase().includes(searchQuery.toLowerCase()); +const matchesSearch = useCallback( + (name: string) => name.toLowerCase().includes(searchQuery.toLowerCase()), + [searchQuery] +);
276-321: Consider extracting the empty state card into a reusable component.The empty state card could be reused across different list views in the application.
+const EmptyStateCard = ({ searchQuery, t }: { searchQuery: string; t: TFunction }) => ( + <Card className="col-span-full"> + <CardContent className="p-6 text-center text-gray-500"> + {searchQuery ? t("no_locations_found") : t("no_locations_available")} + </CardContent> + </Card> +); const renderListView = () => { if (isLoading) { return <TableSkeleton count={6} />; } if (!tableData?.length) { - return ( - <Card className="col-span-full"> - <CardContent className="p-6 text-center text-gray-500"> - {searchQuery ? t("no_locations_found") : t("no_locations_available")} - </CardContent> - </Card> - ); + return <EmptyStateCard searchQuery={searchQuery} t={t} />; } // ... rest of the code };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/pages/Facility/settings/locations/LocationList.tsx (2)
39-56: LGTM! Well-structured hierarchy builder.The
buildLocationHierarchyfunction efficiently organizes locations into a parent-child structure using Maps, which provides O(n) time complexity.
435-440:❓ Verification inconclusive
Consider adding loading state to the LocationSheet.
The
LocationSheetcomponent might need to handle loading states when fetching location details.
🏁 Script executed:
#!/bin/bash # Check if LocationSheet handles loading states ast-grep --pattern 'function LocationSheet({ $_ }) { $$$ const [isLoading, setIsLoading] = useState($$$); $$$ }'Length of output: 120
🏁 Script executed:
#!/bin/bash # Check if LocationSheet is implemented as an arrow function with a loading state. ast-grep --pattern 'const LocationSheet = ({ $_ }) => { $$$ const \[isLoading, setIsLoading\] = useState($$$); $$$ }'Length of output: 125
Attention: Missing Loading Indicator in LocationSheet
Our verification attempts did not find any evidence of a local loading state (e.g., an
isLoadingflag usinguseState) within theLocationSheetcomponent. If the component is expected to fetch or update location details asynchronously, it would benefit from having a defined loading state to improve the user experience while data is being loaded.
- Suggestion: Add a loading state (for example,
const [isLoading, setIsLoading] = useState(false)) and adjust the UI accordingly (e.g., display a spinner or placeholder) during asynchronous operations.- Next Step: Confirm whether the absence of this state is intentional (perhaps managed externally) or if it should be incorporated directly in
LocationSheet.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
76-96: Consider memoizing search-related functions.The search-related functions are recreated on every render. Consider extracting and memoizing them to improve performance.
+const matchesSearch = useCallback( + (name: string, query: string) => + name.toLowerCase().includes(query.toLowerCase()), + [] +); +const hasMatchingDescendant = useCallback( + (locationId: string, allLocations: LocationListType[], query: string): boolean => { + const children = allLocations.filter( + (loc) => loc.parent?.id === locationId, + ); + return children.some( + (child) => matchesSearch(child.name, query) || hasMatchingDescendant(child.id, allLocations, query), + ); + }, + [matchesSearch] +); const filteredData = useMemo(() => { if (!searchQuery) return data?.results || []; const allLocations = data?.results || []; - const matchesSearch = (name: string) => - name.toLowerCase().includes(searchQuery.toLowerCase()); return allLocations.filter( (location) => - matchesSearch(location.name) || hasMatchingDescendant(location.id), + matchesSearch(location.name, searchQuery) || hasMatchingDescendant(location.id, allLocations, searchQuery), ); }, [data?.results, searchQuery, matchesSearch, hasMatchingDescendant]);
155-188: Potential performance issue with search effect.The search effect rebuilds the entire expanded rows state on every search query change. Consider optimizing this by:
- Debouncing the search input
- Using a more efficient state update mechanism
+import { useDebouncedCallback } from 'use-debounce'; +const debouncedSetSearchQuery = useDebouncedCallback( + (value: string) => setSearchQuery(value), + 300 +); // In the Input onChange handler: -setSearchQuery(e.target.value); +debouncedSetSearchQuery(e.target.value);
428-476: Consider extracting the info box into a separate component.The info box markup is quite complex and could be extracted into a reusable component to improve maintainability.
+const LocationInfoBox = () => { + const { t } = useTranslation(); + return ( + <div className="rounded-lg border-2 border-blue-200 bg-blue-50 p-4"> + {/* ... existing info box markup ... */} + </div> + ); +}; // In the main component: {activeTab === "list" && <LocationInfoBox />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/pages/Facility/settings/locations/LocationList.tsx (4)
39-56: LGTM! Efficient hierarchy building implementation.The
buildLocationHierarchyfunction efficiently organizes locations into a parent-child structure using a Map, which provides O(1) lookup time.
67-74: Add error handling for the query.The query could fail due to network issues or server errors, but there's no error handling in place.
const { data, isLoading } = useQuery({ queryKey: ["locations", facilityId], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: {}, }), enabled: !!facilityId, + retry: 2, + onError: (error) => { + // Handle error appropriately + console.error('Failed to fetch locations:', error); + }, });
210-226: Potential performance issue with recursive state updates.The
toggleAllChildrenfunction recursively updates state for all children. For deeply nested structures with many nodes, this could cause performance issues.
479-488: Simplify the conditional rendering of views.The current implementation has commented-out code and a redundant conditional that always renders the list view.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
213-229: 🛠️ Refactor suggestionOptimize recursive state updates in toggleAllChildren.
The current implementation recursively updates state for each child, which could cause performance issues with deeply nested structures.
Consider batching the updates:
const toggleAllChildren = () => { + const getAllChildIds = (parentId: string, expand: boolean): Record<string, boolean> => { + const result: Record<string, boolean> = {}; + getChildren(parentId).forEach((child) => { + result[child.id] = expand; + Object.assign(result, getAllChildIds(child.id, expand)); + }); + return result; + }; + setExpandedRows((prevExpandedRows) => { - const newExpandedRows = { ...prevExpandedRows }; - const toggleChildren = (parentId: string, expand: boolean) => { - getChildren(parentId).forEach((child) => { - newExpandedRows[child.id] = expand; - toggleChildren(child.id, expand); - }); - }; const shouldExpand = !children.every( (child) => prevExpandedRows[child.id], ); - newExpandedRows[location.id] = shouldExpand; - toggleChildren(location.id, shouldExpand); - return newExpandedRows; + return { + ...prevExpandedRows, + [location.id]: shouldExpand, + ...getAllChildIds(location.id, shouldExpand), + }; }); };
🧹 Nitpick comments (2)
src/pages/Facility/settings/locations/LocationList.tsx (2)
76-96: Consider optimizing deep recursion in search logic.The
hasMatchingDescendantfunction recursively traverses the entire location tree for each search. For deep hierarchies, this could impact performance.Consider memoizing intermediate results or using an iterative approach:
-const hasMatchingDescendant = (locationId: string): boolean => { +const hasMatchingDescendant = (locationId: string, cache = new Map<string, boolean>()): boolean => { + if (cache.has(locationId)) return cache.get(locationId)!; + const children = allLocations.filter( (loc) => loc.parent?.id === locationId, ); - return children.some( + const result = children.some( (child) => matchesSearch(child.name) || hasMatchingDescendant(child.id), ); + cache.set(locationId, result); + return result; };
485-494: Simplify the conditional rendering of views.Since the map view is commented out and will be added later, the conditional rendering can be simplified.
-{ - activeTab === "list" - ? renderListView() - : /* Map view will be added later - <div className="h-[600px] bg-gray-100 rounded-lg flex items-center justify-center"> - <p className="text-gray-500">{t("map_view_coming_soon")}</p> - </div> - */ - renderListView() // Default to list view for now -} +{renderListView()}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(6 hunks)src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (2)
39-56: LGTM! Well-structured utility function.The
buildLocationHierarchyfunction efficiently organizes locations into a parent-child structure using a Map, making it easy to traverse the hierarchy.
335-380: LGTM! Well-structured list view implementation.The
renderListViewfunction properly handles all states (loading, empty, populated) and uses semantic table structure for accessibility.public/locale/en.json (1)
579-584: LGTM! Well-structured localization keys.The new localization keys follow consistent naming patterns and properly handle dynamic content with placeholders for icons and text formatting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
213-229: 🛠️ Refactor suggestionOptimize recursive state updates for better performance.
The current implementation of
toggleAllChildrencould cause performance issues with deeply nested structures.This is a duplicate of a previous review comment. The suggestion to batch updates or use a more efficient state management approach still applies.
485-494: 🛠️ Refactor suggestionSimplify the view rendering logic.
The current implementation has commented-out code and a redundant conditional that always renders the list view.
This is a duplicate of a previous review comment. The suggestion to simplify the conditional rendering still applies.
67-74: 🛠️ Refactor suggestionConsider adding error handling and loading states.
The query configuration lacks error handling which could lead to silent failures.
const { data, isLoading } = useQuery({ queryKey: ["locations", facilityId], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: {}, }), enabled: !!facilityId, + retry: 2, + onError: (error) => { + // Handle error appropriately using your error notification system + console.error('Failed to fetch locations:', error); + }, });
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
193-333: Consider decomposing LocationRow into smaller components.The LocationRow component is handling multiple responsibilities including rendering, state management, and child management. Consider breaking it down into smaller, more focused components.
+const LocationActions = ({ location, children, allExpanded, toggleAllChildren, handleEditLocation, isMobile }) => ( + <div className="flex justify-between items-center gap-2"> + {/* ... action buttons ... */} + </div> +); +const LocationName = ({ location, isTopLevel, children, isExpanded, toggleRow }) => ( + <div className="flex items-center"> + {/* ... name display logic ... */} + </div> +); const LocationRow = ({ location, // ... other props }) => { // ... setup code ... return ( <> <TableRow> <TableCell> - {/* ... current implementation ... */} + <LocationName {...nameProps} /> + {isTopLevel && <LocationActions {...actionProps} />} </TableCell> </TableRow> {/* ... children rendering ... */} </> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
335-380: Well-structured list view implementation!The list view implementation handles loading and empty states appropriately, and uses a table layout effectively for structured display.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/components/LocationRow.tsx (1)
76-97: Simplify conditional rendering logic.The nested ternary expressions make the code harder to read. Consider extracting this into a separate function for better maintainability.
- {isTopLevel || children.length > 0 ? ( - <Button - size="icon" - variant="link" - onClick={() => toggleRow(location.id)} - disabled={children.length === 0} - > - {isExpanded ? ( - <CareIcon icon="l-angle-down" className="h-5 w-5" /> - ) : ( - <CareIcon icon="l-angle-right" className="h-5 w-5" /> - )} - </Button> - ) : location.parent ? ( - <CareIcon - icon="l-corner-down-right-alt" - className="h-4 w-4 text-gray-400 ml-4 mr-2" - /> - ) : ( - <div className="w-8" /> - )} + {renderExpandButton()}Add this helper function:
const renderExpandButton = () => { if (isTopLevel || children.length > 0) { return ( <Button size="icon" variant="link" onClick={() => toggleRow(location.id)} disabled={children.length === 0} > <CareIcon icon={isExpanded ? "l-angle-down" : "l-angle-right"} className="h-5 w-5" /> </Button> ); } if (location.parent) { return ( <CareIcon icon="l-corner-down-right-alt" className="h-4 w-4 text-gray-400 ml-4 mr-2" /> ); } return <div className="w-8" />; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)src/pages/Facility/settings/locations/components/LocationRow.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (7)
src/pages/Facility/settings/locations/components/LocationRow.tsx (3)
15-25: Well-structured props interface!The props interface is well-defined with clear types and follows TypeScript best practices.
43-59: Optimize recursive state updates for better performance.The recursive state updates in
toggleAllChildrencould impact performance with deeply nested structures.
148-160: LGTM! Recursive rendering is well implemented.The recursive rendering of child rows with proper prop passing ensures consistent behavior across the hierarchy.
src/pages/Facility/settings/locations/LocationList.tsx (4)
35-52: Excellent implementation of location hierarchy!The
buildLocationHierarchyfunction efficiently organizes locations using a Map for O(1) lookups, making it performant for large datasets.
72-92: Optimize search and filtering logic to reduce redundancy.Multiple implementations of similar filtering logic could impact performance with large datasets.
Also applies to: 100-108
189-236: Well-implemented list view with proper state handling!The
renderListViewfunction effectively handles loading states, empty results, and table layout.
341-350: Simplify the conditional rendering of views.The current implementation has commented-out code and a redundant conditional that always renders the list view.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
72-125: 🛠️ Refactor suggestionOptimize search and filtering logic to reduce code duplication.
The code has multiple implementations of the same search logic (
matchesSearchfunction) and similar filtering patterns. This could impact performance and maintainability.+const createSearchMatcher = (searchQuery: string) => { + const query = searchQuery.toLowerCase(); + return (name: string) => name.toLowerCase().includes(query); +}; +const createHierarchySearcher = (allLocations: LocationListType[], matchesSearch: (name: string) => boolean) => { + const hasMatchingDescendant = (locationId: string): boolean => { + const children = allLocations.filter((loc) => loc.parent?.id === locationId); + return children.some((child) => + matchesSearch(child.name) || hasMatchingDescendant(child.id) + ); + }; + return hasMatchingDescendant; +}; const filteredData = useMemo(() => { if (!searchQuery) return data?.results || []; const allLocations = data?.results || []; - const matchesSearch = (name: string) => - name.toLowerCase().includes(searchQuery.toLowerCase()); + const matchesSearch = createSearchMatcher(searchQuery); + const hasMatchingDescendant = createHierarchySearcher(allLocations, matchesSearch); return allLocations.filter( (location) => matchesSearch(location.name) || hasMatchingDescendant(location.id), ); }, [data?.results, searchQuery]);
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
35-52: Add error handling to buildLocationHierarchy function.The function should handle potential edge cases such as circular references in the parent-child relationships.
function buildLocationHierarchy(locations: LocationListType[]) { const childrenMap = new Map<string, LocationListType[]>(); const topLevelLocations: LocationListType[] = []; + const seenIds = new Set<string>(); locations.forEach((location) => { + // Check for circular references + if (seenIds.has(location.id)) { + console.warn(`Circular reference detected for location ${location.id}`); + return; + } + seenIds.add(location.id); + if (!location.parent || Object.keys(location.parent).length === 0) { topLevelLocations.push(location); } else {
142-152: Optimize toggleRow to avoid unnecessary re-renders.The function modifies state directly and triggers multiple state updates. Consider using a functional update and batching the changes.
const toggleRow = (id: string) => { - const newExpandedRows = { ...expandedRows }; - newExpandedRows[id] = !newExpandedRows[id]; - const children = getChildren(id); - children.forEach((child) => { - if (!child.has_children) { - newExpandedRows[child.id] = !newExpandedRows[child.id]; - } - }); - setExpandedRows(newExpandedRows); + setExpandedRows((prev) => { + const next = { ...prev }; + next[id] = !next[id]; + getChildren(id) + .filter(child => !child.has_children) + .forEach(child => { + next[child.id] = next[id]; + }); + return next; + }); };
242-364: Enhance accessibility for the location management interface.Consider adding ARIA labels and roles to improve screen reader support.
- <Page title={t("locations")} hideTitleOnPage={true} className="p-0"> + <Page + title={t("locations")} + hideTitleOnPage={true} + className="p-0" + role="region" + aria-label={t("locations_management")} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
63-70: Add error handling for the query.The query could fail due to network issues or server errors, but there's no error handling in place.
177-186: Handle potential null values safely.The parent lookup logic could lead to runtime errors if the location is not found.
345-354: Simplify the conditional rendering of views.The current implementation has commented-out code and a redundant conditional that always renders the list view.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/pages/Facility/settings/locations/components/LocationListView.tsx (4)
53-69: Consider optimizing recursive state updates.The
toggleAllChildrenfunction recursively updates state for all children. For deeply nested structures with many nodes, this could cause performance issues.Consider batching the updates:
const toggleAllChildren = () => { - setExpandedRows((prevExpandedRows) => { + const getAllChildIds = (parentId: string, expand: boolean, ids: Record<string, boolean> = {}) => { const newExpandedRows = { ...prevExpandedRows }; - const toggleChildren = (parentId: string, expand: boolean) => { - getChildren(parentId).forEach((child) => { - newExpandedRows[child.id] = expand; - toggleChildren(child.id, expand); - }); - }; + getChildren(parentId).forEach((child) => { + ids[child.id] = expand; + getAllChildIds(child.id, expand, ids); + }); + return ids; + }; + + setExpandedRows((prevExpandedRows) => { const shouldExpand = !children.every( (child) => prevExpandedRows[child.id], ); - newExpandedRows[location.id] = shouldExpand; - toggleChildren(location.id, shouldExpand); - return newExpandedRows; + return { + ...prevExpandedRows, + [location.id]: shouldExpand, + ...getAllChildIds(location.id, shouldExpand), + }; }); };
141-149: Improve accessibility for interactive elements.The link and button elements need proper ARIA labels for better screen reader support.
-<Button variant="white" size={isMobile ? "xs" : "sm"} asChild> +<Button + variant="white" + size={isMobile ? "xs" : "sm"} + asChild + aria-label={t("see_location_details", { name: location.name })} +> <Link href={`/location/${location.id}`} className="text-gray-900 flex items-center" + role="link" >
202-204: Enhance loading state feedback.The current loading state uses a basic skeleton loader. Consider adding more context about what's being loaded.
-return <TableSkeleton count={6} />; +return ( + <div role="status" aria-live="polite"> + <p className="sr-only">{t("loading_locations")}</p> + <TableSkeleton count={6} /> + </div> +);
206-214: Improve empty state messaging.The empty state message could be more helpful by providing guidance on next steps.
return ( <Card className="col-span-full"> <CardContent className="p-6 text-center text-gray-500"> - {searchQuery ? t("no_locations_found") : t("no_locations_available")} + <p> + {searchQuery + ? t("no_locations_found_with_query", { query: searchQuery }) + : t("no_locations_available_add_one")} + </p> + {!searchQuery && ( + <Button + variant="link" + onClick={onAddLocation} + className="mt-2" + > + {t("add_first_location")} + </Button> + )} </CardContent> </Card> );src/pages/Facility/settings/locations/LocationList.tsx (2)
54-61: Consider implementing pagination or virtual scrolling.Loading all locations at once could impact performance with large datasets. Consider implementing pagination or virtual scrolling for better performance.
const { data, isLoading } = useQuery({ queryKey: ["locations", facilityId], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, - queryParams: {}, + queryParams: { + limit: 50, + offset: 0, + }, }), enabled: !!facilityId, + onError: (error) => { + console.error('Failed to fetch locations:', error); + }, });
145-182: Simplify search expansion logic.The search effect is complex and could be simplified. Consider using a more declarative approach.
useEffect(() => { if (!searchQuery) { setExpandedRows({}); return; } - const allLocations = data?.results || []; - const matchesSearch = (name: string) => - name.toLowerCase().includes(searchQuery.toLowerCase()); - - const hasMatchingDescendant = (locationId: string): boolean => { - const children = allLocations.filter( - (loc) => loc.parent?.id === locationId, - ); - return children.some( - (child) => matchesSearch(child.name) || hasMatchingDescendant(child.id), - ); - }; + const matchingLocations = filteredData.filter( + location => createSearchMatcher(searchQuery)(location.name) + ); const newExpandedRows: Record<string, boolean> = {}; - allLocations.forEach((location) => { - if (matchesSearch(location.name) || hasMatchingDescendant(location.id)) { - let currentLoc = location; - while (currentLoc.parent?.id) { - newExpandedRows[currentLoc.parent.id] = true; - const parentLoc = allLocations.find( - (loc) => loc.id === currentLoc.parent?.id, - ); - if (!parentLoc) { - break; - } - currentLoc = parentLoc; - } + matchingLocations.forEach(location => { + let currentLoc = location; + while (currentLoc.parent?.id) { + newExpandedRows[currentLoc.parent.id] = true; + currentLoc = data?.results.find( + loc => loc.id === currentLoc.parent?.id + ) ?? currentLoc; } }); setExpandedRows(newExpandedRows); -}, [searchQuery, data?.results]); +}, [searchQuery, filteredData, data?.results]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)src/pages/Facility/settings/locations/components/LocationListView.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
59-66: 🛠️ Refactor suggestionAdd error handling to the query configuration.
The query lacks error handling and retry configuration, which could lead to poor user experience during network issues.
const { data, isLoading } = useQuery({ queryKey: ["locations", facilityId], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: {}, }), enabled: !!facilityId, + retry: 2, + onError: (error) => { + // Handle error appropriately, e.g., show toast notification + console.error('Failed to fetch locations:', error); + }, + staleTime: 5 * 60 * 1000, // Cache data for 5 minutes });
🧹 Nitpick comments (3)
src/pages/Facility/settings/locations/LocationList.tsx (3)
31-48: Optimize the hierarchy building function for better performance.The
buildLocationHierarchyfunction could be optimized to reduce iterations and Map lookups.function buildLocationHierarchy(locations: LocationListType[]) { - const childrenMap = new Map<string, LocationListType[]>(); - const topLevelLocations: LocationListType[] = []; + const result = locations.reduce((acc, location) => { + if (!location.parent || Object.keys(location.parent).length === 0) { + acc.topLevelLocations.push(location); + } else { + const parentId = location.parent.id; + if (!acc.childrenMap.has(parentId)) { + acc.childrenMap.set(parentId, []); + } + acc.childrenMap.get(parentId)!.push(location); + } + return acc; + }, { + childrenMap: new Map<string, LocationListType[]>(), + topLevelLocations: [] as LocationListType[] + }); - locations.forEach((location) => { - if (!location.parent || Object.keys(location.parent).length === 0) { - topLevelLocations.push(location); - } else { - const parentId = location.parent.id; - if (!childrenMap.has(parentId)) { - childrenMap.set(parentId, []); - } - childrenMap.get(parentId)?.push(location); - } - }); - return { childrenMap, topLevelLocations }; + return result; }
139-149: Improve the row toggle handler for better type safety and immutability.The
toggleRowfunction mutates state directly and lacks type safety.const toggleRow = (id: string) => { - const newExpandedRows = { ...expandedRows }; - newExpandedRows[id] = !newExpandedRows[id]; - const children = getChildren(id); - children.forEach((child) => { - if (!child.has_children) { - newExpandedRows[child.id] = !newExpandedRows[child.id]; - } - }); - setExpandedRows(newExpandedRows); + setExpandedRows((prev) => { + const childUpdates = getChildren(id) + .filter((child) => !child.has_children) + .reduce((acc, child) => ({ + ...acc, + [child.id]: !prev[child.id] + }), {}); + + return { + ...prev, + [id]: !prev[id], + ...childUpdates + }; + }); };
208-215: Remove commented-out map view code.The commented-out map view code should be removed as it's not currently used and clutters the codebase. If needed, this code can be retrieved from version control when implementing the map view feature.
- {/* Map view will be added later - <TabsTrigger value="map" id="location-map-view"> - <div className="flex items-center gap-1"> - <CareIcon icon="l-map" className="text-lg" /> - <span>{t("map")}</span> - </div> - </TabsTrigger> - */} - {/* Map view will be added later, for now always show list view */}Also applies to: 293-293
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
Proposed Changes
Desktop view
Mobile view
Added new search feature
Added client side filtering and auto expand on search as we are already fetching all the data using pagination.
Screen.Recording.2025-02-19.at.9.19.01.PM.mov
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit