Encounter Locations view #10969
Encounter Locations view #10969
Conversation
…location-view-for-patients
…location-view-for-patients
WalkthroughThis pull request introduces enhancements to localization and routing functionalities. It adds multiple new key-value pairs to the JSON locale file and introduces several new components: an encounter information card, an overview component with tabbed navigation for patient and location views, and improved location management components with state and data fetching logic. Additionally, the patient routing logic has been restructured to support dynamic redirection and route parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router
participant EO as EncountersOverview
participant EL as EncounterList
participant LL as LocationList
U->>R: Navigate to /facility/{facilityId}/encounters
R->>R: Redirect to /facility/{facilityId}/encounters/patients
R->>EO: Render EncountersOverview ("patients" tab)
EO->>U: Display tabbed interface (patients, locations)
U->>EO: Select "locations" tab
EO->>R: Navigate to /facility/{facilityId}/encounters/locations
R->>EO: Render EncountersOverview ("locations" tab)
EO->>LL: Render LocationList component
EO->>U: Display updated location content
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
86b4316
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://836f9179.care-fe.pages.dev |
| Branch Preview URL: | https://location-view-for-patients.care-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/components/ui/sidebar/facility-nav.tsx (1)
37-41: Consider using a location-specific icon instead of "d-patient"The new "locations" link is currently using the "d-patient" icon, which is also used for patient-related navigation items. For better visual differentiation and user clarity, it would be better to use a more appropriate and distinct icon for locations (e.g., "d-map-pin" or something location-related if available).
{ name: t("locations"), url: `${baseUrl}/locations`, - icon: "d-patient", + icon: "d-map-pin", // or another location-specific icon },src/components/Encounter/EncounterInfoCard.tsx (2)
27-53: Consider enhancing utility functions with TypeScript enums and constants.The utility functions
getStatusColorandgetPriorityColoruse string literals for status and priority values. Consider enhancing type safety by:
- Using TypeScript enums or union types for status and priority values
- Creating constants for the color classes to make maintenance easier
+ type EncounterStatus = 'planned' | 'in_progress' | 'completed' | 'cancelled'; + type EncounterPriority = 'stat' | 'urgent' | 'asap' | 'routine'; - const getStatusColor = (status: string) => { + const getStatusColor = (status: EncounterStatus) => { switch (status) { case "planned": return "bg-blue-100 text-blue-800"; // other cases... } }; - const getPriorityColor = (priority: string) => { + const getPriorityColor = (priority: EncounterPriority) => { // function body... }
55-120: Component implements good UI/UX practices with appropriate accessors.The component is well-structured with:
- Proper data extraction from props
- Conditional rendering of patient death status
- Semantic badges with appropriate colors
- Accessible navigation links with clear visual affordances
- Use of i18n for all user-facing strings
One suggestion for improved internationalization:
The "View Details" text on line 113 should use the translation function:
- View Details + {t("view_details")}src/pages/Facility/locations/LocationList.tsx (5)
203-209: Scroll-into-view might cause a jarring experience for large trees.
Automatically scrolling to the newly selected node is handy, but it can be disorienting when many locations expand. If performance or user flow becomes an issue, consider optimizing with a condition-based scroll or virtualization.
214-223: Consider adding pagination or dynamic loading for child locations.
The hard-codedlimit: 100might miss scenarios where more than 100 children exist. If you anticipate large sets of child locations, implement pagination or load-on-demand for better scalability.
292-342: Unify color scheme and consider accessibility for occupancy status.
The bed status styling is clear, but ensure these color contrasts meet accessibility requirements. If future theming or brand guidelines change, extracting these statuses into a shared style or theme provider is beneficial.
526-538: Evaluate necessity of useMemo in LocationSummary.
These calculations are relatively modest. If performance is not a pressing concern, you may omituseMemo. On the flip side, if you handle large facility data, continuing this optimization makes sense.
569-708: Add test coverage for the main LocationList component and subcomponents.
Given the complexity of queries, expansions, and multiple states, unit or integration tests would help maintain reliability.Would you like a starter test suite covering these features?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
public/locale/en.json(8 hunks)src/Routers/AppRouter.tsx(2 hunks)src/Routers/routes/LocationRoutes.tsx(1 hunks)src/Routers/routes/PatientRoutes.tsx(2 hunks)src/components/Encounter/EncounterInfoCard.tsx(1 hunks)src/components/ui/sidebar/facility-nav.tsx(1 hunks)src/pages/Encounters/EncounterList.tsx(5 hunks)src/pages/Encounters/EncountersOverview.tsx(1 hunks)src/pages/Facility/locations/LocationList.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (29)
src/Routers/routes/LocationRoutes.tsx (1)
1-11: Implementation looks goodThe LocationRoutes module follows the established pattern for route definitions in the application. The route correctly passes the facilityId parameter to the LocationList component.
src/Routers/AppRouter.tsx (2)
27-27: Integration of LocationRoutes looks correctThe import for LocationRoutes follows the same pattern as other route imports.
54-54: Proper integration of LocationRoutesThe LocationRoutes are correctly merged into the main Routes object, following the established pattern for other route modules.
src/Routers/routes/PatientRoutes.tsx (3)
19-21: Redirection implementation looks goodThe redirection from
/facility/:facilityId/encountersto/facility/:facilityId/encounters/patientsfollows best practices for route restructuring. This approach ensures that existing links to the encounters page will still work correctly.
22-24:✅ Verification successful
Verify that EncountersOverview handles tab parameter correctly
The new route passes both
facilityIdandtabas props to the EncountersOverview component. Ensure that this component is properly designed to handle different tab values, particularly the default "patients" tab that users will be redirected to.
🏁 Script executed:
#!/bin/bash # Check how EncountersOverview handles the tab parameter # Look at the implementation of EncountersOverview fd "EncountersOverview.tsx" src/ --exec cat {} | grep -A 15 "interface" || fd "EncountersOverview.tsx" src/ --exec cat {}Length of output: 366
EncountersOverview properly handles the tab parameter
The component correctly sets a default value of"patients"for thetabprop and processes it as expected. No changes are needed in this context.
12-12:❓ Verification inconclusive
Verify removal of EncounterList import
The import of EncounterList has been replaced with EncountersOverview. Ensure that any code that relied on the direct import of EncounterList has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Check for other imports of EncounterList that might need updating # Look for imports of EncounterList across the codebase rg "import .*EncounterList" src/ --type tsx --type tsLength of output: 83
Below is an updated review comment. It notes that while the change to import EncountersOverview in place of EncounterList looks correct in PatientRoutes.tsx, further verification should be performed (using an updated script command) to ensure no residual imports of EncounterList remain in the codebase.
Verify that all lingering references to EncounterList are removed
- The code in src/Routers/routes/PatientRoutes.tsx now imports:
import EncountersOverview from "@/pages/Encounters/EncountersOverview";- Please ensure that no other files still import or refer to EncounterList. The initial search using
rgwith file type flags resulted in an error. Re-run the following script to verify that no instances exist in.tsor.tsxfiles:# Re-run the search for any remaining imports of EncounterList rg "import .*EncounterList" src/ -g "*.tsx" -g "*.ts"- If this script produces no output, then the replacement is complete. Otherwise, please update any files still referencing EncounterList.
src/components/Encounter/EncounterInfoCard.tsx (2)
1-20: Well-structured imports and component organization.The imports are well-organized with a logical grouping: third-party libraries first, followed by local utils, icons, UI components, and types.
21-25: Clean interface definition with appropriate prop types.The interface is well-defined with clear prop types. The
hideBorderoptional prop with a boolean type provides flexibility for different styling contexts.src/pages/Encounters/EncountersOverview.tsx (4)
1-9: Appropriate imports with clear organization.The imports are well-structured with a logical grouping.
10-13: Well-defined props interface.The interface is concise and properly defines the expected props for the component.
25-59: Well-structured component with good tab navigation pattern.The component implements a clean tab navigation pattern with:
- Proper value binding between tab state and UI
- Clear navigation implementation
- Responsive layout with appropriate styling
- Proper use of i18n for tab labels
- Consistent content height management
51-57:❓ Verification inconclusive
Verify LocationList component compatibility.
Ensure the
LocationListcomponent is designed to accept and properly use thefacilityIdprop. Check that it's implemented to handle encounter locations and not just generic facility locations.
🏁 Script executed:
#!/bin/bash # Check the LocationList component implementation to ensure it handles facilityId correctly echo "Checking LocationList component implementation..." rg -A 3 "interface.*LocationList.*Props" --type ts rg -A 3 "function LocationList" --type ts rg -A 5 "facilityId.*LocationList" --type ts echo "Done."Length of output: 1776
Verify that the correct LocationList component is tailored for encounter-specific needs
It appears that both LocationList implementations accept a
facilityIdprop. However, please double-check that the version imported intoEncountersOverview.tsxis intentionally designed to filter or adjust for encounter locations (vs. showing generic facility locations). If the behavior isn’t encounter-specific, consider updating the logic or selecting a more appropriate component.src/pages/Encounters/EncounterList.tsx (4)
39-42: Updated prop type signature is more appropriate.Making
facilityIda required prop is a good practice since it's essential for the component's functionality. This change makes the API more explicit about dependencies.
129-130: Updated call to buildQueryParams with correct parameter order.This line has been properly updated to account for the changed parameter order in the
buildQueryParamsfunction.
658-664: Good refactoring to use the new EncounterInfoCard component.The code has been properly refactored to use the new
EncounterInfoCardcomponent, which should improve consistency and maintainability. The props are correctly passed to the new component.
44-66:✅ Verification successful
Updated parameter order in buildQueryParams function.
The function now takes
facilityIdas the first parameter, which is a good change since it's the primary identifier required for querying encounters.Let's verify that all places where this function is called have been updated to match the new parameter order:
🏁 Script executed:
#!/bin/bash # Check all usage locations of buildQueryParams to ensure they've been updated echo "Checking buildQueryParams usage..." rg -A 1 "buildQueryParams" --type ts echo "Done."Length of output: 431
Parameter Order Update Verified
The updated parameter order forbuildQueryParams(placingfacilityIdas the first parameter) has been verified. The function call insrc/pages/Encounters/EncounterList.tsxnow correctly reflects this update:
- Verified Call:
buildQueryParams(facilityId, status, encounterClass, priority)No further changes are necessary.
public/locale/en.json (9)
458-458: Appropriate translation key added for bed availability.This translation key will be useful for showing availability stats in the locations view.
957-957: Key translation added for encounter locations feature.This is a critical translation key for the new "Encounter Locations" tab in the EncountersOverview component.
1538-1539: Translation key for empty child locations state.This will be shown when a location has no child locations, providing a clear user message.
1546-1546: Translation key for missing location descriptions.This is a good fallback text when a location doesn't have a description.
1653-1653: Translation key for occupied beds metric.This complements the "available_beds" key to show both sides of bed occupancy statistics.
1714-1714: Translation key for patient encounters tab.This will be used for the "Patients" tab in the EncountersOverview component, providing a descriptive label.
1908-1908: Translation key for admission readiness status.This key will likely be used to indicate patients or beds that are prepared for admission.
2104-2104: Translation key for location search functionality.This will provide appropriate text for the search interface in the LocationList component.
2343-2343: Translation key for location totals.This will be used to display the total count of locations, likely in a statistics view.
src/pages/Facility/locations/LocationList.tsx (4)
82-93: Watch out for potential cyclical references in location hierarchy.
While it's unlikely in a well-formed dataset, consider safeguardinggetParentChain()against unexpected cycles or malformed data, which could cause infinite loops.
95-176: UseCallback dependencies look good, but verify concurrency assumptions.
YouruseLocationState()logic appears solid. All callbacks correctly use functional state updates, which helps avoid stale references. However, ensure no external states or props outside of these callbacks need to be addressed as dependencies for stable concurrency.
427-432: Confirm the parent update logic from the first child only.
Reassigning parent data by reading only the first child (children?.results?.[0]) might be valid for your use case, but double-check scenarios with multiple children or missing parents to avoid unexpected behaviors.
669-671: Revisit disabling search outside selected location.
Disabling the search bar when no location is selected prevents users from quickly finding top-level locations. Confirm whether this limitation matches user needs or if global search is preferred.
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
location-view-for-patients
|
| Run status |
|
| Run duration | 07m 17s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
12
|
| View all changes introduced in this branch ↗︎ | |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…location-view-for-patients
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/pages/Encounters/EncountersOverview.tsx (5)
1-8: Imports organized clearly. Consider adding potential mobile optimization imports.The imports are well structured, separating external libraries from internal components. If this component will be used on mobile devices, you might want to consider adding a hook like useMediaQuery for responsive layout adjustments.
14-19: Component structure looks good. Consider adding error handling.The component follows React functional component patterns correctly. The default value for the
tabprop is appropriately defined.Consider adding error handling for cases where facilityId might be invalid:
export default function EncountersOverview({ facilityId, tab = "patients", }: EncountersOverviewProps) { const { t } = useTranslation(); + + if (!facilityId) { + return <div className="p-4">{t("common:error.invalidFacilityId")}</div>; + }
25-27: Navigation logic is correct. Consider adding state loading indication.The navigation logic to switch between tabs is implemented correctly.
Consider adding a loading state when switching tabs to improve user experience:
onValueChange={(value) => { + setIsLoading(true); navigate(`/facility/${facilityId}/encounters/${value}`); }}You would need to add a useState hook at the top of the component:
const [isLoading, setIsLoading] = useState(false);
29-44: Tab UI implementation looks good. Consider accessibility improvements.The tabs UI is well structured using the UI library components with proper translation support.
Consider enhancing accessibility by adding ARIA attributes:
<TabsList className="bg-transparent p-0 h-8" aria-label={t("encounters.tabsLabel")}>
20-56: Component rendering logic is well-structured. Consider memoization for performance.The overall component structure and rendering logic is clean and follows React best practices.
For improved performance, especially if this component might re-render frequently, consider wrapping it with React.memo:
-export default function EncountersOverview({ +function EncountersOverview({ facilityId, tab = "patients", }: EncountersOverviewProps) { // Component code... } +export default React.memo(EncountersOverview);This would require adding an import for React:
+import React from "react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/sidebar/facility-nav.tsx(1 hunks)src/pages/Encounters/EncountersOverview.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/sidebar/facility-nav.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (3)
src/pages/Encounters/EncountersOverview.tsx (3)
9-12: Well-defined interface with appropriate typing.The interface is concise and clear with proper TypeScript typing. The optional tab parameter with default value in the function definition follows best practices.
46-48: Patient tab content implementation is correct.The patient tab content is implemented correctly, passing the required facilityId prop to the EncounterList component.
50-52: Location tab content implementation is correct.The location tab content is implemented correctly, passing the required facilityId prop to the LocationList component.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/pages/Facility/locations/LocationList.tsx (4)
426-433: Refine parent location update logic.
The side effect updates the parent location data after fetching children but only uses the first child's parent. If multiple children exist, or if the parent reference is missing, you may end up with partial or unintended data in state. Consider explicitly verifying or clarifying which parent to use.
523-567: Display a fallback when no locations are loaded inLocationSummary.
If an API error occurs or data is unexpectedly empty, the component silently returnsnull. That can confuse users. Consider showing a small message or skeleton to guide users.
582-591: Add error UI for top-level location queries.
The top-level locations (allLocations) also lack explicit error handling. NeglectingisErrorcan leave the user unaware of failed data loading. Adding a fallback or alert message is recommended.
642-707: Improve test coverage for new functionalities.
This file introduces multiple new UI features (e.g., hierarchical display, search, breadcrumbs, location summaries). To maintain code quality, add unit/integration tests to verify each component’s behavior, especially queries, expansions, and search filters.Would you like help drafting these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/locations/LocationList.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/locations/LocationList.tsx (1)
292-343: Validate occupancy logic for beds.
Your approach to identifying occupied beds by checkinglocation.current_encounteris clear, but it might be prone to errors if future data structures evolve. Ensure thatlocation.current_encounteris guaranteed to represent occupancy in all long-term scenarios, or add defensive checks for partial data.
| // Hook for location data management | ||
| function useLocationState(): LocationState & { | ||
| handleLocationSelect: (location: LocationListType) => void; | ||
| handleToggleExpand: (locationId: string) => void; | ||
| handleSearchChange: (value: string) => void; | ||
| handleLocationData: (location: LocationListType | undefined) => void; | ||
| } { | ||
| const [state, setState] = useState<LocationState>({ | ||
| selectedLocationId: null, | ||
| selectedLocation: null, | ||
| expandedLocations: new Set(), | ||
| searchQuery: "", | ||
| }); | ||
|
|
||
| const handleLocationSelect = useCallback((location: LocationListType) => { | ||
| if (!location.id) { | ||
| setState((prev) => ({ | ||
| ...prev, | ||
| selectedLocationId: null, | ||
| selectedLocation: null, | ||
| searchQuery: "", | ||
| })); | ||
| return; | ||
| } | ||
|
|
||
| const parentIds = getParentChain(location); | ||
| setState((prev) => ({ | ||
| ...prev, | ||
| selectedLocationId: location.id, | ||
| selectedLocation: location, | ||
| searchQuery: "", | ||
| expandedLocations: new Set([...prev.expandedLocations, ...parentIds]), | ||
| })); | ||
| }, []); | ||
|
|
||
| const handleToggleExpand = useCallback((locationId: string) => { | ||
| setState((prev) => { | ||
| const next = new Set(prev.expandedLocations); | ||
| if (next.has(locationId)) { | ||
| next.delete(locationId); | ||
| } else { | ||
| next.add(locationId); | ||
| } | ||
| return { ...prev, expandedLocations: next }; | ||
| }); | ||
| }, []); | ||
|
|
||
| const handleSearchChange = useCallback((value: string) => { | ||
| setState((prev) => ({ ...prev, searchQuery: value })); | ||
| }, []); | ||
|
|
||
| const handleLocationData = useCallback( | ||
| (location: LocationListType | undefined) => { | ||
| if (!location) return; | ||
|
|
||
| setState((prev) => { | ||
| if ( | ||
| !prev.selectedLocation || | ||
| location.id === prev.selectedLocationId || | ||
| prev.selectedLocation.id === location.id | ||
| ) { | ||
| return prev; | ||
| } | ||
| return { | ||
| ...prev, | ||
| selectedLocation: { | ||
| ...prev.selectedLocation, | ||
| parent: location, | ||
| }, | ||
| }; | ||
| }); | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
| return { | ||
| ...state, | ||
| handleLocationSelect, | ||
| handleToggleExpand, | ||
| handleSearchChange, | ||
| handleLocationData, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Introduce error handling for useQuery operations.
Currently, none of the useQuery calls in the useLocationState hook handle the error state. If the request fails, the user might not receive any feedback or fallback UI. Consider extending the pattern you use for isLoading to display or log errors for a more robust user experience.
Do you need a sample approach to display errors within this hook or in the associated components?
| const { data: children, isLoading } = useQuery({ | ||
| queryKey: ["locations", facilityId, "children", location.id], | ||
| queryFn: query(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| parent: location.id, | ||
| limit: 100, | ||
| mode: "kind", | ||
| }, | ||
| }), | ||
| enabled: isExpanded && hasChildren, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle query failure states for child locations.
You are handling isLoading but ignoring the scenario where isError is true. Without error feedback, data-fetch failures may leave the UI in an ambiguous state.
const { data: children, isLoading, isError } = useQuery({
...
});
+ if (isError) {
+ return (
+ <div className="p-4 text-red-500">
+ {t("failed_to_load_child_locations")}
+ </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, isLoading } = useQuery({ | |
| queryKey: ["locations", facilityId, "children", location.id], | |
| queryFn: query(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: location.id, | |
| limit: 100, | |
| mode: "kind", | |
| }, | |
| }), | |
| enabled: isExpanded && hasChildren, | |
| }); | |
| const { data: children, isLoading, isError } = useQuery({ | |
| queryKey: ["locations", facilityId, "children", location.id], | |
| queryFn: query(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: location.id, | |
| limit: 100, | |
| mode: "kind", | |
| }, | |
| }), | |
| enabled: isExpanded && hasChildren, | |
| }); | |
| if (isError) { | |
| return ( | |
| <div className="p-4 text-red-500"> | |
| {t("failed_to_load_child_locations")} | |
| </div> | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/pages/Facility/locations/LocationList.tsx (3)
94-176:⚠️ Potential issueHandle error states in useQuery operations consistently.
The useLocationState hook uses React Query via multiple useQuery calls, but none of them handle potential error states. This could lead to a poor user experience if API requests fail.
I recommend implementing error handling for all React Query operations to provide feedback to users when data fetching fails. Here's how you might enhance the useLocationState hook to include error state handling and provide appropriate UI fallbacks where needed.
// This would need to be applied to each useQuery call in the component const { data: data, isLoading, isError, error } = useQuery({ // ... existing query config }); + // Handle error state in the component rendering + if (isError) { + return ( + <div className="p-4 text-red-500"> + {t("error_loading_data")}: {error instanceof Error ? error.message : String(error)} + </div> + ); + }
211-223:⚠️ Potential issueAdd error handling for child location queries.
You're checking for isLoading but not handling potential query failures.
Add error state handling to provide feedback when location data can't be loaded:
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: ["locations", facilityId, "children", location.id], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location.id, limit: 100, mode: "kind", }, }), enabled: isExpanded && hasChildren, }); + // Add error handling in the component rendering + if (isError) { + return ( + <div className="text-xs text-red-500 ml-6"> + {t("failed_to_load_child_locations")} + </div> + ); + }
414-424:⚠️ Potential issueImplement error handling for SelectedLocationChildren query.
The component currently only handles the loading state but not potential query failures.
Add error state handling to provide user feedback when location data can't be loaded:
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: ["locations", facilityId, "children", selectedLocationId, "full"], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: selectedLocationId, limit: 100, }, }), enabled: !!selectedLocationId, }); + if (isError) { + return ( + <Card className="col-span-full"> + <CardContent className="p-6 text-center text-red-500"> + {t("error_loading_child_locations")} + </CardContent> + </Card> + ); + }
🧹 Nitpick comments (8)
src/pages/Facility/locations/LocationList.tsx (8)
667-675: Add debounce to search input for performance optimization.The current implementation updates the search query on every keystroke, which could lead to excessive rendering.
Implement debouncing for the search input to improve performance:
+ import { useDebounce } from '@/hooks/useDebounce'; // Create this hook or use a library // Inside your component: + const [searchInputValue, setSearchInputValue] = useState(""); + const debouncedSearchValue = useDebounce(searchInputValue, 300); + // Update search query when debounced value changes + useEffect(() => { + handleSearchChange(debouncedSearchValue); + }, [debouncedSearchValue, handleSearchChange]); // Then update the input: <Input placeholder={t("search_by_name")} - value={searchQuery} - onChange={(e) => handleSearchChange(e.target.value)} + value={searchInputValue} + onChange={(e) => setSearchInputValue(e.target.value)} className="w-full" disabled={!selectedLocationId} />
292-343: Enhance BedCard for better accessibility and interactive feedback.The BedCard component could benefit from improved accessibility and user interaction cues.
Consider these improvements for the BedCard component:
function BedCard({ location, facilityId }: BedCardProps) { const { t } = useTranslation(); const isOccupied = !!location.current_encounter; + // Add click handler if location has an encounter + const handleCardClick = () => { + if (location.current_encounter) { + // Navigate to encounter details or show modal + // Example: navigate(`/facility/${facilityId}/encounter/${location.current_encounter.id}`); + } + }; return ( <div + role={isOccupied ? "button" : undefined} + tabIndex={isOccupied ? 0 : undefined} + onClick={isOccupied ? handleCardClick : undefined} + onKeyDown={(e) => { + if (isOccupied && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + handleCardClick(); + } + }} className={`border rounded-lg overflow-hidden shadow-sm h-fit ${ isOccupied ? "bg-white border-gray-200" : "bg-green-50 border-green-200" + }${isOccupied ? " hover:shadow-md transition-all cursor-pointer" : ""}`} + aria-label={`${location.name} - ${isOccupied ? t("occupied") : t("available")}`} > {/* Rest of component */} </div> ); }
605-610: Optimize the topLevelLocations filter for better performance.The current implementation might unnecessarily re-filter the locations on each render.
Use a more explicit check and add proper memoization:
const topLevelLocations = useMemo(() => { if (!allLocations?.results) return []; return allLocations.results.filter( - (loc) => !loc.parent || Object.keys(loc.parent).length === 0, + (loc) => !loc.parent?.id, // More explicit check for parent ID ); - }, [allLocations?.results]); + }, [allLocations?.results?.length]); // More precise dependency
582-603: Consider implementing pagination for large location datasets.The current implementation loads all locations at once with a fixed limit (QUERY_LIMIT), which might not scale well for facilities with many locations.
For better scalability, consider implementing pagination or virtualized lists for large datasets. You might also want to implement a more advanced search feature that uses backend filtering rather than client-side filtering:
// In the query, add pagination parameters const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ queryKey: ["locations", facilityId, "all"], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mine: true, page_size: QUERY_LIMIT, + // Add search term to backend query if available + search: searchQuery.length > 2 ? searchQuery : undefined, }, }), + // Re-fetch when search query changes if using backend search + enabled: !searchQuery || searchQuery.length > 2, }); // Then add pagination controls in the UI + const [page, setPage] = useState(1); + // Add pagination controls in the UI
612-642: Enhance responsiveness for mobile devices.While there is some mobile responsiveness with
hidden md:blockfor the sidebar, the overall layout could be improved for small screens.Consider adding a mobile-friendly navigation option:
<div className="flex px-4 space-x-4 h-[calc(100vh-4rem)]"> {/* Left sidebar - Location tree */} {topLevelLocations.length > 0 && ( + <> + {/* Mobile view - Location selector */} + <div className="w-full md:hidden mb-4"> + <div className="bg-white rounded-lg shadow p-4"> + <label className="block text-sm font-medium mb-2">{t("select_location")}</label> + <select + className="w-full border border-gray-300 rounded-md" + value={selectedLocationId || ""} + onChange={(e) => { + const locId = e.target.value; + if (!locId) { + handleLocationSelect({ id: "", name: "" } as LocationListType); + } else { + const selectedLoc = allLocations?.results.find(l => l.id === locId); + if (selectedLoc) handleLocationSelect(selectedLoc); + } + }} + > + <option value="">{t("all_locations")}</option> + {allLocations?.results.map(loc => ( + <option key={loc.id} value={loc.id}>{loc.name}</option> + ))} + </select> + </div> + </div> <div className="w-64 shadow-lg bg-white rounded-lg hidden md:block"> {/* Existing sidebar code */} </div> + </> )}
596-601: Add retry logic for failed queries.In case of network issues or API failures, it would be helpful to have automatic retry logic for important queries.
Add retry options to critical queries:
const { data: selectedChildren } = useQuery({ queryKey: ["locations", facilityId, "children", selectedLocationId, "full"], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: selectedLocationId, limit: 100, }, }), enabled: !!selectedLocationId, + retry: 3, // Retry failed queries 3 times + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), // Exponential backoff });
1-43: Consider organizing imports for better readability.The current import section is quite lengthy and could be better organized.
Consider grouping imports by type (React, UI components, utilities, etc.) for improved readability:
// React and third-party libraries import { useQuery } from "@tanstack/react-query"; import React, { useCallback, useMemo, useState, useEffect } from "react"; import { useTranslation } from "react-i18next"; // Icons import { ArrowRight, Bed, // ... other icons } from "lucide-react"; // UI components import { Badge } from "@/components/ui/badge"; import { // ... other UI components } from "@/components/ui/breadcrumb"; // Application components import { CardGridSkeleton } from "@/components/Common/SkeletonLoading"; import EncounterInfoCard from "@/components/Encounter/EncounterInfoCard"; // Utilities and types import query from "@/Utils/request/query"; import { LocationList as LocationListType } from "@/types/location/location"; import locationApi from "@/types/location/locationApi";
44-72: Consider using TypeScript enums or unions for location form types.The current approach of using
keyof typeof LocationIconworks but could be more type-safe.Consider using a union type for location forms:
- type LocationFormType = keyof typeof LocationIcon; + // Define location form types as a union + type LocationFormType = + | 'si' | 'bu' | 'wi' | 'wa' | 'lvl' | 'co' + | 'ro' | 'bd' | 've' | 'ho' | 'ca' | 'rd' + | 'area' | 'jdn' | 'vi'; // Then use this type consistently const LocationIcon: Record<LocationFormType, React.FC<LucideProps>> = { si: Building2, // Site // ... other mappings }This approach provides better type safety and autocompletion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(9 hunks)src/components/ui/sidebar/facility-nav.tsx(1 hunks)src/pages/Facility/locations/LocationList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/sidebar/facility-nav.tsx
- public/locale/en.json
⏰ 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: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
| const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ | ||
| queryKey: ["locations", facilityId, "all"], | ||
| queryFn: query.paginated(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| mine: true, | ||
| page_size: QUERY_LIMIT, | ||
| }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Add error handling for the main locations query.
The query for all locations doesn't handle potential error states.
Implement error handling for a better user experience:
- const { data: allLocations, isLoading: isLoadingLocations } = useQuery({
+ const {
+ data: allLocations,
+ isLoading: isLoadingLocations,
+ isError: isErrorLocations,
+ error: locationsError
+ } = useQuery({
queryKey: ["locations", facilityId, "all"],
queryFn: query.paginated(locationApi.list, {
pathParams: { facility_id: facilityId },
queryParams: {
mine: true,
page_size: QUERY_LIMIT,
},
}),
});
// Add in the appropriate section of the render function:
+ if (isErrorLocations) {
+ return (
+ <div className="flex items-center justify-center h-[calc(100vh-4rem)]">
+ <Card className="w-full max-w-md">
+ <CardContent className="p-6 text-center">
+ <div className="text-red-500 mb-2">
+ {t("failed_to_load_locations")}
+ </div>
+ <Button onClick={() => window.location.reload()}>
+ {t("retry")}
+ </Button>
+ </CardContent>
+ </Card>
+ </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: allLocations, isLoading: isLoadingLocations } = useQuery({ | |
| queryKey: ["locations", facilityId, "all"], | |
| queryFn: query.paginated(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| mine: true, | |
| page_size: QUERY_LIMIT, | |
| }, | |
| }), | |
| }); | |
| const { | |
| data: allLocations, | |
| isLoading: isLoadingLocations, | |
| isError: isErrorLocations, | |
| error: locationsError | |
| } = useQuery({ | |
| queryKey: ["locations", facilityId, "all"], | |
| queryFn: query.paginated(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| mine: true, | |
| page_size: QUERY_LIMIT, | |
| }, | |
| }), | |
| }); | |
| if (isErrorLocations) { | |
| return ( | |
| <div className="flex items-center justify-center h-[calc(100vh-4rem)]"> | |
| <Card className="w-full max-w-md"> | |
| <CardContent className="p-6 text-center"> | |
| <div className="text-red-500 mb-2"> | |
| {t("failed_to_load_locations")} | |
| </div> | |
| <Button onClick={() => window.location.reload()}> | |
| {t("retry")} | |
| </Button> | |
| </CardContent> | |
| </Card> | |
| </div> | |
| ); | |
| } |
| function Breadcrumbs({ | ||
| location, | ||
| onSelect, | ||
| t, | ||
| }: { | ||
| location: LocationListType; | ||
| onSelect: (location: LocationListType) => void; | ||
| t: (key: string) => string; | ||
| }) { | ||
| const items = []; | ||
| let current: LocationListType | undefined = location; | ||
|
|
||
| while (current) { | ||
| items.unshift(current); | ||
| current = current.parent; | ||
| } | ||
|
|
||
| return ( | ||
| <Breadcrumb> | ||
| <BreadcrumbList> | ||
| {items.map((item, index) => ( | ||
| <React.Fragment key={item.id}> | ||
| {index > 0 && <BreadcrumbSeparator />} | ||
| <BreadcrumbItem> | ||
| <BreadcrumbLink | ||
| className={`hover:text-primary cursor-pointer ${ | ||
| index === items.length - 1 ? "font-medium text-primary" : "" | ||
| }`} | ||
| onClick={() => | ||
| onSelect( | ||
| index === 0 | ||
| ? ({ id: "", name: "" } as LocationListType) | ||
| : item, | ||
| ) | ||
| } | ||
| > | ||
| {index === 0 ? t("locations") : item.name} | ||
| </BreadcrumbLink> | ||
| </BreadcrumbItem> | ||
| </React.Fragment> | ||
| ))} | ||
| </BreadcrumbList> | ||
| </Breadcrumb> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix the type casting in Breadcrumbs when selecting the root level.
The current implementation creates an incomplete LocationListType object when navigating to the root level.
The code is casting an incomplete object to LocationListType, which might cause runtime errors. Consider creating a proper root location object:
- onClick={() =>
- onSelect(
- index === 0
- ? ({ id: "", name: "" } as LocationListType)
- : item,
- )
- }
+ onClick={() => {
+ if (index === 0) {
+ // Create a root-level location object with all required properties
+ const rootLocation: LocationListType = {
+ id: "",
+ name: t("locations"),
+ form: "si", // Default form
+ has_children: true,
+ // Add other required properties with default values
+ parent: undefined,
+ availability_status: "available",
+ description: "",
+ facility: facility_id,
+ };
+ onSelect(rootLocation);
+ } else {
+ onSelect(item);
+ }
+ }}Committable suggestion skipped: line range outside the PR's diff.
…location-view-for-patients
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/pages/Facility/locations/LocationList.tsx (3)
225-264: 🛠️ Refactor suggestionRevisit error handling for child location queries.
ThisuseQuerycall manages the children of a location but omitsisErrorchecks. Previously, reviewers suggested displaying an error state to enhance user feedback.const { data: children, isLoading } = useQuery({ ... + isError, + error }); + if (isError) { + return ( + <div className="p-4 text-red-500"> + {t("failed_to_load_child_locations")} + </div> + ); + }
434-460: 🛠️ Refactor suggestionAdd error handling for
SelectedLocationChildrenquery.
The query for children pages also lacksisErrorchecks. Without them, users receive no feedback when an error occurs.
570-580: 🛠️ Refactor suggestionImplement fallback UI for the main locations query.
You handle loading states but notisErrororerrorfromuseQuery. Returning an error message or a retry option would provide clarity if loading fails.
🧹 Nitpick comments (11)
public/locale/en.json (10)
458-458: Ensure consistent casing across localization entries.
"Available beds" is capitalized differently than entries like "Available Beds" or "Ready For Admission." Consider adjusting for a uniform user-facing experience.
959-959: Maintain uniform casing for consistency.
"Encounter Locations" uses Title Case, which is fine, but verify similar entries (e.g., "Search Locations" vs "search locations") to keep them consistent throughout.
1532-1532: Check phrasing alignment.
"No Child Locations" is Title Cased. Compare with related entries (e.g., "No locations" vs "No Child Locations") and choose a standard approach.
1550-1550: Consider matching capitalization style with other "No..." entries.
Currently: "No Description" is Title Cased. If you prefer sentence case (e.g., "No description"), align it accordingly.
1570-1570: Align case style with "No Child Locations."
"No locations" has a lowercase second word, whereas "No Child Locations" is Title Cased. Consider standardizing.
1658-1658: Apply consistent casing to match "Ordered" or "Lowercase" usage.
"Occupied beds" is partially Title Cased. Consider "Occupied Beds" if that aligns better with other headings.
1720-1720: Avoid spaces in the localization key.
Currently, the key is"patient encounters". Most other keys are underscored (e.g.,"encounter_locations"). Using"patient_encounters"might ensure consistency.
1913-1913: Confirm adherence to conventional casing.
"Ready For Admission" uses Title Case. If your design system prefers sentence case, consider "Ready for admission."
2119-2119: Casing consistency.
"Search Locations" is Title Cased. Ensure it aligns with related keys (e.g., "search_locations" or "Search locations") for uniform appearance.
2349-2349: Standardize capitalization for user-facing text.
"Total locations" might be changed to "Total Locations," matching the style of "Encounter Locations" and others.src/pages/Facility/locations/LocationList.tsx (1)
1-689: Consider unit tests foruseLocationState.
This hook’s logic (e.g.,handleSearchChange,handlePageChange) is critical to location management. Adding tests can improve confidence in future refactorings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(9 hunks)src/pages/Facility/locations/LocationList.tsx(1 hunks)
⏰ 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)
|
@amjithtitus09 is it ready for testing, iam seeing a lot of commit after the labelling |
|
@nihal467 Ready for testing |
|
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 (3)
src/pages/Facility/locations/LocationList.tsx (3)
226-237: 🛠️ Refactor suggestionAdd error handling for child location queries.
The current implementation only handles the loading state but doesn't account for query failures, which may leave users without proper feedback.
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: ["locations", facilityId, "children", location.id], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location.id, mode: "kind", }, pageSize: 100, }), enabled: isExpanded, // Only fetch when the node is expanded }); + // Add after line 238, before the return statement + if (isError) { + return ( + <div className="pl-2 py-1 text-sm text-red-500"> + {t("failed_to_load_child_locations")} + </div> + ); + }
615-625: 🛠️ Refactor suggestionAdd error handling for the main locations query.
The query for all locations doesn't handle potential error states.
- const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ + const { + data: allLocations, + isLoading: isLoadingLocations, + isError: isErrorLocations, + } = useQuery({ queryKey: ["locations", facilityId, "all"], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mine: true, mode: "kind", }, pageSize: 100, }), }); + // Add in the return JSX, as a condition before rendering the main content + if (isErrorLocations) { + return ( + <div className="flex items-center justify-center h-[calc(100vh-4rem)]"> + <Card className="w-full max-w-md"> + <CardContent className="p-6 text-center"> + <div className="text-red-500 mb-2"> + {t("failed_to_load_locations")} + </div> + <Button onClick={() => window.location.reload()}> + {t("retry")} + </Button> + </CardContent> + </Card> + </div> + ); + }
435-455: 🛠️ Refactor suggestionHandle error state for location children query.
Similar to the tree node query, this query doesn't handle error states, which could lead to a confusing user experience if the API request fails.
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: [ "locations", facilityId, "children", selectedLocationId, "full", currentPage, searchQuery, ], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: selectedLocationId, limit: ITEMS_PER_PAGE, offset: (currentPage - 1) * ITEMS_PER_PAGE, name: searchQuery, }, }), enabled: !!selectedLocationId, }); + // Add after line 464, before the isLoading check + if (isError) { + return ( + <Card className="col-span-full"> + <CardContent className="p-6 text-center text-red-500"> + {t("failed_to_load_child_locations")} + </CardContent> + </Card> + ); + }
🧹 Nitpick comments (4)
src/pages/Facility/locations/LocationList.tsx (4)
349-354: Add null check before rendering EncounterInfoCard.While
isOccupiedchecks for the existence oflocation.current_encounter, there's no explicit null check before passing it to the EncounterInfoCard component.- <EncounterInfoCard - encounter={location.current_encounter} - facilityId={facilityId} - hideBorder={true} - /> + {location.current_encounter && ( + <EncounterInfoCard + encounter={location.current_encounter} + facilityId={facilityId} + hideBorder={true} + /> + )}
617-625: Consider implementing pagination or lazy loading for large facility locations.The current implementation fetches 100 top-level locations at once, which might cause performance issues for large facilities.
Consider implementing a more efficient data loading strategy such as:
- Reducing the initial page size and implementing lazy loading
- Using pagination for top-level locations similar to how you've done it for child locations
- Implementing a virtual list for better performance with large data sets
479-493: Use destructuring for cleaner array reduction.The current code could be simplified using object destructuring in the reducer callback.
const { bedLocations, nonBedLocations } = children.results.reduce( - (acc, location) => { + ({ bedLocations, nonBedLocations }, location) => { if (location.form === "bd") { - acc.bedLocations.push(location); + bedLocations.push(location); } else { - acc.nonBedLocations.push(location); + nonBedLocations.push(location); } - return acc; + return { bedLocations, nonBedLocations }; }, { bedLocations: [] as LocationListType[], nonBedLocations: [] as LocationListType[], }, );
600-728: The component lacks a loading indicator for the initial render.Users might be confused if the component shows nothing before locations are loaded.
Consider adding a loading indicator for the initial state:
return ( + <> + {isLoadingLocations && !allLocations && ( + <div className="flex items-center justify-center h-[calc(100vh-4rem)]"> + <div className="h-8 w-8 animate-spin rounded-full border-4 border-gray-300 border-t-gray-600" /> + </div> + )} + {(!isLoadingLocations || allLocations) && ( <div className="flex px-4 space-x-4 min-h-[100vh]"> {/* Rest of the component */} </div> + )} + </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(10 hunks)src/pages/Facility/locations/LocationList.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 (8)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (2)
src/pages/Facility/locations/LocationList.tsx (2)
582-588: Fix the type casting in Breadcrumbs when selecting the root level.The current implementation creates an incomplete LocationListType object when navigating to the root level.
The code is casting an incomplete object to LocationListType, which might cause runtime errors. Consider creating a proper root location object with all required properties.
307-358: Consider sorting bed cards based on availability status.According to the PR comments, there was feedback to sort beds to ensure availability of the top bed is always displayed.
You might want to implement sorting logic at the parent component level (SelectedLocationChildren) before passing the data to BedCard:
// In SelectedLocationChildren component, line ~480 // Sort bedLocations to show available beds first + bedLocations.sort((a, b) => { + // If a is available and b is occupied, a comes first + if (!a.current_encounter && b.current_encounter) return -1; + // If a is occupied and b is available, b comes first + if (a.current_encounter && !b.current_encounter) return 1; + // Otherwise maintain the original order + return 0; + });
@nihal467 This is not a bug since the user will be able to see only the child locations they have access to even in the parent's view |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/pages/Facility/locations/LocationList.tsx (4)
226-236: 🛠️ Refactor suggestionImplement error handling for the location tree query.
The query for fetching child locations in
LocationTreeNodeonly handles the loading state but doesn't handle potential errors.- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: ["locations", facilityId, "children", location.id], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location.id, mode: "kind", }, pageSize: 100, }), enabled: isExpanded, // Only fetch when the node is expanded }); + // Add this inside the return after line 247 + if (isError) { + return ( + <div className="text-red-500 text-sm pl-4"> + {t("failed_to_load_child_locations")} + </div> + ); + }
435-455: 🛠️ Refactor suggestionHandle query failure states for child locations.
You are handling
isLoadingbut ignoring the scenario whereisErroris true. Without error feedback, data-fetch failures may leave the UI in an ambiguous state.- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: [ "locations", facilityId, "children", selectedLocationId, "full", currentPage, searchQuery, ], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: selectedLocationId, limit: ITEMS_PER_PAGE, offset: (currentPage - 1) * ITEMS_PER_PAGE, name: searchQuery, }, }), enabled: !!selectedLocationId, }); + // Add this after isLoading check on line 465 + if (isError) { + return ( + <div className="p-4 text-red-500"> + {t("failed_to_load_child_locations")} + </div> + ); + }
615-625: 🛠️ Refactor suggestionAdd error handling for the main locations query.
The query for all locations doesn't handle potential error states.
- const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ + const { + data: allLocations, + isLoading: isLoadingLocations, + isError: isErrorLocations, + error: locationsError + } = useQuery({ queryKey: ["locations", facilityId, "all"], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mine: true, mode: "kind", }, pageSize: 100, }), }); // Add in the appropriate section of the render function: + if (isErrorLocations) { + return ( + <div className="flex items-center justify-center h-[calc(100vh-4rem)]"> + <Card className="w-full max-w-md"> + <CardContent className="p-6 text-center"> + <div className="text-red-500 mb-2"> + {t("failed_to_load_locations")} + </div> + <Button onClick={() => window.location.reload()}> + {t("retry")} + </Button> + </CardContent> + </Card> + </div> + ); + }
583-588:⚠️ Potential issueFix the type casting in Breadcrumbs when selecting the root level.
The current implementation creates an incomplete LocationListType object when navigating to the root level.
- onClick={() => - onSelect( - index === 0 - ? ({ id: "", name: "" } as LocationListType) - : item, - ) - } + onClick={() => { + if (index === 0) { + // Create a root-level location object with all required properties + const rootLocation: LocationListType = { + id: "", + name: t("locations"), + form: "si", // Default form + has_children: true, + // Add other required properties with default values + parent: undefined, + availability_status: "available", + description: "", + facility: facilityId, + }; + onSelect(rootLocation); + } else { + onSelect(item); + } + }}
🧹 Nitpick comments (4)
src/pages/Facility/locations/LocationList.tsx (4)
679-683: Add translation for search placeholder.The placeholder string "search_by_name" should have a proper translation key.
<div className="w-full sm:w-72 shrink-0"> <Input - placeholder={t("search_by_name")} + placeholder={t("search_locations")} value={searchQuery} onChange={(e) => handleSearchChange(e.target.value)} className="w-full" disabled={!selectedLocationId} />
342-347: Add more meaningful content for available beds.Empty beds display minimal content. Consider adding action buttons or more details for better usability.
{!location.current_encounter ? ( <div className="flex flex-col items-center justify-center py-4 h-auto"> <p className="text-sm text-gray-600 mb-3"> {t("ready_for_admission")} </p> + <Button size="sm" className="mt-2"> + {t("admit_patient")} + </Button> </div> ) : (
415-431: Simplify props interface for SelectedLocationChildren.The component defines its props interface inline, making it harder to reuse. Consider extracting this to a named interface type.
+interface SelectedLocationChildrenProps { + facilityId: string; + selectedLocationId: string; + onSelect: (location: LocationListType) => void; + searchQuery: string; + onLocationData: (location: LocationListType | undefined) => void; + currentPage: number; + onPageChange: (page: number) => void; +} function SelectedLocationChildren({ facilityId, selectedLocationId, onSelect, searchQuery, onLocationData, currentPage, onPageChange, -}: { - facilityId: string; - selectedLocationId: string; - onSelect: (location: LocationListType) => void; - searchQuery: string; - onLocationData: (location: LocationListType | undefined) => void; - currentPage: number; - onPageChange: (page: number) => void; -}) { +}: SelectedLocationChildrenProps) {
480-493: Consider sorting beds by availability for better user experience.Beds are currently displayed in the order they're returned from the API. Consider sorting them to show available beds first, as mentioned in the PR comments.
// Group locations by type (bed vs non-bed) const { bedLocations, nonBedLocations } = children.results.reduce( (acc, location) => { if (location.form === "bd") { acc.bedLocations.push(location); } else { acc.nonBedLocations.push(location); } return acc; }, { bedLocations: [] as LocationListType[], nonBedLocations: [] as LocationListType[], }, ); + // Sort beds by availability (available first) + bedLocations.sort((a, b) => { + // If a is available and b is not, a comes first + if (!a.current_encounter && b.current_encounter) return -1; + // If b is available and a is not, b comes first + if (a.current_encounter && !b.current_encounter) return 1; + // Otherwise maintain original order + return 0; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/locations/LocationList.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
| function ChildLocationCard(props: ChildLocationCardProps) { | ||
| const isBed = props.location.form === "bd"; | ||
| return isBed ? ( | ||
| <BedCard {...props} onClick={undefined} /> | ||
| ) : ( | ||
| <LocationCard {...props} /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix onClick propagation in ChildLocationCard for beds.
The current implementation nullifies onClick for beds, breaking the ability to select them in the location hierarchy.
function ChildLocationCard(props: ChildLocationCardProps) {
const isBed = props.location.form === "bd";
return isBed ? (
- <BedCard {...props} onClick={undefined} />
+ <BedCard {...props} onClick={props.onClick} />
) : (
<LocationCard {...props} />
);
}📝 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.
| function ChildLocationCard(props: ChildLocationCardProps) { | |
| const isBed = props.location.form === "bd"; | |
| return isBed ? ( | |
| <BedCard {...props} onClick={undefined} /> | |
| ) : ( | |
| <LocationCard {...props} /> | |
| ); | |
| } | |
| function ChildLocationCard(props: ChildLocationCardProps) { | |
| const isBed = props.location.form === "bd"; | |
| return isBed ? ( | |
| <BedCard {...props} onClick={props.onClick} /> | |
| ) : ( | |
| <LocationCard {...props} /> | |
| ); | |
| } |
| function BedCard({ location, facilityId }: BedCardProps) { | ||
| const { t } = useTranslation(); | ||
| const isOccupied = !!location.current_encounter; | ||
|
|
||
| return ( | ||
| <div | ||
| className={`border rounded-lg overflow-hidden shadow-sm h-full ${ | ||
| isOccupied ? "bg-white border-gray-200" : "bg-green-50 border-green-200" | ||
| }`} | ||
| > | ||
| <div | ||
| className={`px-4 py-3 flex justify-between items-center ${ | ||
| isOccupied | ||
| ? "bg-blue-50 border-b border-blue-100" | ||
| : "bg-green-100 border-b border-green-200" | ||
| }`} | ||
| > | ||
| <div className="flex items-center"> | ||
| <Bed | ||
| className={`h-4 w-4 mr-2 ${isOccupied ? "text-blue-600" : "text-green-600"}`} | ||
| /> | ||
| <span className="font-medium">{location.name}</span> | ||
| </div> | ||
| <div | ||
| className={`text-xs px-2 py-1 rounded-full ${ | ||
| isOccupied | ||
| ? "bg-blue-100 text-blue-800" | ||
| : "bg-green-200 text-green-800" | ||
| }`} | ||
| > | ||
| {isOccupied ? t("occupied") : t("available")} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div> | ||
| {!location.current_encounter ? ( | ||
| <div className="flex flex-col items-center justify-center py-4 h-auto"> | ||
| <p className="text-sm text-gray-600 mb-3"> | ||
| {t("ready_for_admission")} | ||
| </p> | ||
| </div> | ||
| ) : ( | ||
| <EncounterInfoCard | ||
| encounter={location.current_encounter} | ||
| facilityId={facilityId} | ||
| hideBorder={true} | ||
| /> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The BedCard component should handle onClick events for beds.
The BedCard component receives an onClick prop but it doesn't use it. This means beds cannot be selected by clicking, although they're displayed as selectable items in the ChildLocationCard hierarchy.
return (
<div
+ onClick={onClick}
className={`border rounded-lg overflow-hidden shadow-sm h-full ${
isOccupied ? "bg-white border-gray-200" : "bg-green-50 border-green-200"
- }`}
+ } cursor-pointer hover:shadow-md transition-shadow`}
>
<div
className={`px-4 py-3 flex justify-between items-center ${📝 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.
| function BedCard({ location, facilityId }: BedCardProps) { | |
| const { t } = useTranslation(); | |
| const isOccupied = !!location.current_encounter; | |
| return ( | |
| <div | |
| className={`border rounded-lg overflow-hidden shadow-sm h-full ${ | |
| isOccupied ? "bg-white border-gray-200" : "bg-green-50 border-green-200" | |
| }`} | |
| > | |
| <div | |
| className={`px-4 py-3 flex justify-between items-center ${ | |
| isOccupied | |
| ? "bg-blue-50 border-b border-blue-100" | |
| : "bg-green-100 border-b border-green-200" | |
| }`} | |
| > | |
| <div className="flex items-center"> | |
| <Bed | |
| className={`h-4 w-4 mr-2 ${isOccupied ? "text-blue-600" : "text-green-600"}`} | |
| /> | |
| <span className="font-medium">{location.name}</span> | |
| </div> | |
| <div | |
| className={`text-xs px-2 py-1 rounded-full ${ | |
| isOccupied | |
| ? "bg-blue-100 text-blue-800" | |
| : "bg-green-200 text-green-800" | |
| }`} | |
| > | |
| {isOccupied ? t("occupied") : t("available")} | |
| </div> | |
| </div> | |
| <div> | |
| {!location.current_encounter ? ( | |
| <div className="flex flex-col items-center justify-center py-4 h-auto"> | |
| <p className="text-sm text-gray-600 mb-3"> | |
| {t("ready_for_admission")} | |
| </p> | |
| </div> | |
| ) : ( | |
| <EncounterInfoCard | |
| encounter={location.current_encounter} | |
| facilityId={facilityId} | |
| hideBorder={true} | |
| /> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| } | |
| function BedCard({ location, facilityId, onClick }: BedCardProps) { | |
| const { t } = useTranslation(); | |
| const isOccupied = !!location.current_encounter; | |
| return ( | |
| <div | |
| onClick={onClick} | |
| className={`border rounded-lg overflow-hidden shadow-sm h-full ${ | |
| isOccupied ? "bg-white border-gray-200" : "bg-green-50 border-green-200" | |
| } cursor-pointer hover:shadow-md transition-shadow`} | |
| > | |
| <div | |
| className={`px-4 py-3 flex justify-between items-center ${ | |
| isOccupied | |
| ? "bg-blue-50 border-b border-blue-100" | |
| : "bg-green-100 border-b border-green-200" | |
| }`} | |
| > | |
| <div className="flex items-center"> | |
| <Bed | |
| className={`h-4 w-4 mr-2 ${isOccupied ? "text-blue-600" : "text-green-600"}`} | |
| /> | |
| <span className="font-medium">{location.name}</span> | |
| </div> | |
| <div | |
| className={`text-xs px-2 py-1 rounded-full ${ | |
| isOccupied | |
| ? "bg-blue-100 text-blue-800" | |
| : "bg-green-200 text-green-800" | |
| }`} | |
| > | |
| {isOccupied ? t("occupied") : t("available")} | |
| </div> | |
| </div> | |
| <div> | |
| {!location.current_encounter ? ( | |
| <div className="flex flex-col items-center justify-center py-4 h-auto"> | |
| <p className="text-sm text-gray-600 mb-3"> | |
| {t("ready_for_admission")} | |
| </p> | |
| </div> | |
| ) : ( | |
| <EncounterInfoCard | |
| encounter={location.current_encounter} | |
| facilityId={facilityId} | |
| hideBorder={true} | |
| /> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| } |
|
LGTM, other than the below mentioned issue
|
| const topLevelLocations = allLocations?.results || []; | ||
|
|
||
| return ( | ||
| <div className="flex px-4 space-x-4 min-h-[100vh]"> |
There was a problem hiding this comment.
@bodhish This happens when min-h-full or no height mentioned. We need it to cover the full height right?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/pages/Facility/locations/LocationNavbar.tsx (2)
85-96: 🛠️ Refactor suggestionAdd error handling for the location children query.
You're handling the loading state but not error scenarios. If the query fails, users won't receive any feedback.
- const { data: children, isLoading } = useQuery({ + const { data: children, isLoading, isError } = useQuery({ queryKey: ["locations", facilityId, "children", location.id], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location.id, mode: "kind", }, pageSize: 100, }), enabled: isExpanded, }); + // Add after the return statement's opening + if (isError) { + return ( + <div className="space-y-1"> + <div className={cn( + "flex items-center py-1 px-2 rounded-md cursor-pointer", + isSelected && "bg-blue-100 text-blue-800", + )} + style={{ paddingLeft: `${level * 1}rem` }}> + <div className="flex items-center flex-1 text-sm gap-2 text-red-500"> + <Icon className="h-4 w-4" /> + <span className="truncate">{location.name}</span> + <span className="text-xs">{t("error_loading")}</span> + </div> + </div> + </div> + ); + }
173-183: 🛠️ Refactor suggestionAdd error handling for the main locations query.
The query for all locations doesn't handle potential error states.
- const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ + const { + data: allLocations, + isLoading: isLoadingLocations, + isError: isErrorLocations + } = useQuery({ queryKey: ["locations", facilityId, "all"], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mine: true, mode: "kind", }, pageSize: 100, }), }); // Add before return statement: + if (isErrorLocations) { + return ( + <div className="w-64 shadow-lg bg-white rounded-lg hidden md:block p-4"> + <h2 className="text-lg font-semibold text-red-500">{t("error_loading_locations")}</h2> + </div> + ); + }src/pages/Facility/locations/LocationList.tsx (1)
136-142: 🛠️ Refactor suggestionAdd error handling for location detail query.
The location detail query doesn't handle potential error states. If the API request fails, users won't receive any feedback.
- const { data: locationDetail } = useQuery({ + const { data: locationDetail, isError, error } = useQuery({ queryKey: ["location", facilityId, locationId], queryFn: query(locationApi.get, { pathParams: { facility_id: facilityId, id: locationId }, }), enabled: !!locationId, }); + // Add to the component, before the return: + if (isError && locationId) { + console.error("Failed to load location details:", error); + // Optionally show an error message to the user + }
🧹 Nitpick comments (6)
src/pages/Encounters/EncountersOverview.tsx (1)
31-31: Fix className whitespace.There's an extra leading space in the className that should be removed.
- <div className=" w-fit px-4 py-2 rounded-lg"> + <div className="w-fit px-4 py-2 rounded-lg">src/pages/Facility/locations/LocationList.tsx (1)
145-155: Potential dependency issue in useEffect.The effect depends on
handleLocationSelectwhich could change between renders, potentially causing unnecessary updates or infinite loops.- }, [locationDetail, handleLocationSelect]); + }, [locationDetail, locationId]);Alternatively, use a more stable approach by storing the transformation logic in a ref or by memoizing the
handleLocationSelectfunction with more stable dependencies.src/pages/Facility/locations/LocationContent.tsx (4)
1-22: Add error handling for query failures
Currently, this code handles loading states (viaisLoading) but does not appear to check for or display errors from theuseQueryhook. Consider handling theisErrororerrorstate for improved user experience and troubleshooting.const { data: children, isLoading, isError, error } = useQuery({ ... }); return ( <> - {isLoading ? ( + {isLoading ? ( ... ) : !children?.results?.length ? ( ... + ) : isError ? ( + <Card> + <CardContent className="p-6 text-center text-red-500"> + {error instanceof Error ? error.message : "An unknown error occurred"} + </CardContent> + </Card> ) : ( ... )} </> );
68-124: Improve color accessibility for occupancy states
Consider verifying color contrast (e.g., text on green background) to ensure that users with visual impairments can easily distinguish available/occupied states.
126-175: Enhance accessibility for clickable cards
Using a clickable<div>may hinder keyboard accessibility and screen reader usage. Consider semantic HTML by using a<button>or a role-based element to make the card accessible.-function LocationCard({ location, onClick }: LocationCardProps) { +function LocationCard({ location, onClick }: LocationCardProps) { ... - return ( - <div - className="border rounded-lg overflow-hidden shadow-sm hover:shadow-md transition-shadow cursor-pointer" - onClick={onClick} - > + return ( + <button + type="button" + className="border rounded-lg overflow-hidden shadow-sm hover:shadow-md transition-shadow cursor-pointer text-left" + onClick={onClick} + aria-label={`Open location: ${location.name}`} + > ... - </div> + </button> ); }
242-398: Optional pagination & search refinements
The current logic for pagination and search is functional. As an optional improvement, consider lazy-loading large datasets or using virtualization if the number of location items grows large to enhance performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Routers/routes/PatientRoutes.tsx(2 hunks)src/pages/Encounters/EncountersOverview.tsx(1 hunks)src/pages/Facility/locations/LocationContent.tsx(1 hunks)src/pages/Facility/locations/LocationList.tsx(1 hunks)src/pages/Facility/locations/LocationNavbar.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (9)
src/pages/Encounters/EncountersOverview.tsx (1)
15-19: LGTM! Good design with optional props and defaults.The component is structured well with appropriate default values, making it flexible for different usage scenarios.
src/Routers/routes/PatientRoutes.tsx (1)
19-35: LGTM! Clean routing implementation.The routing changes effectively support the new tabbed navigation pattern, with good use of the Redirect component and proper parameter passing.
src/pages/Facility/locations/LocationNavbar.tsx (1)
192-192:❓ Verification inconclusive
Consider accessibility on mobile devices.
The component is hidden on mobile (
hidden md:block), but there's no alternative navigation method visible for small screens.
🏁 Script executed:
#!/bin/bash # Check if there's mobile navigation elsewhere in the codebase echo "Searching for mobile location navigation components..." rg -A 3 -B 3 "LocationNavbar|location.*navigation" --type tsx | grep -i "mobile\|sm:\|md:\|lg:"Length of output: 244
Below is an updated shell script to re-run our search using a glob pattern instead of the unrecognized file type. This script looks for any components or code hints that suggest an alternative mobile navigation (for example, components named “MobileNavigation” or keywords like “nav” combined with “mobile”) across TSX files:
#!/bin/bash echo "Searching TSX files for potential mobile navigation alternative components..." rg -A 3 -B 3 "MobileNavigation" -g "*.tsx" echo "Searching TSX files for usage of navigation components with mobile qualifiers..." rg -A 3 -B 3 "nav.*mobile|mobile.*nav|menu" -g "*.tsx" | grep -i "mobile\|sm:\|md:\|lg:"Please run this script to gather further details on whether a mobile-friendly alternative exists. Once verified:
- If an alternative mobile navigation component is found: You can confirm that the hidden LocationNavbar is intentional and acceptable.
- If no alternative is found: Consider revisiting the design to ensure mobile users have an accessible navigation option.
**
src/pages/Facility/locations/LocationList.tsx (2)
158-158: Reconsider using min-h-[100vh] for layout.Using
min-h-[100vh]might cause unnecessary scrolling. Consider using a more appropriate height constraint.- <div className="flex px-4 space-x-4 min-h-[100vh]"> + <div className="flex px-4 space-x-4 h-full">
148-152:✅ Verification successful
Verify assumption about location children.
When transforming location detail to list format, you're hardcoding
has_children: false, which might be incorrect if the location actually has children.
🏁 Script executed:
#!/bin/bash # Check if LocationDetail type includes information about children echo "Checking LocationDetail type definition..." rg "interface LocationDetail" --type ts -A 20 echo "Checking for API responses that might include child information..." rg "children|has_children" --type ts | grep -i "location.*detail"Length of output: 1594
Confirmed: Default mapping for location children is appropriate.
Analysis shows that the
LocationDetailtype (located insrc/types/location/location.ts) does not include any children information, whereas theLocationListtype defines thehas_childrenfield as a boolean. Given that the detail view lacks children-related data, defaultinghas_childrentofalsewhen converting a detail to a list is correct for now. If future API changes provide children information within the detail view, this transformation logic should be revisited.src/pages/Facility/locations/LocationContent.tsx (4)
41-66: Use of fallback icon is good
ReturningBuilding2as a fallback icon whenformis missing or unrecognized is a safe approach that prevents runtime errors.
177-190: ChildLocationCard logic looks clear
The conditional render for beds vs. non-bed locations is straightforward. It's easy to maintain and extend.
192-229: Breadcrumb generation is well-structured
Building the hierarchy in reverse and rendering usingBreadcrumbItemis concise and clear. No concerns found here.
231-241: Interfaces are well-defined
Defining explicit prop interfaces clarifies component expectations, improving readability and maintainability.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…location-view-for-patients
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/pages/Facility/locations/LocationContent.tsx (2)
73-105: Consider extracting occupant-specific styling into constants.Although the bed occupancy styling is clear, extracting these style-related strings into constants or a small helper would improve readability and reduce the risk of typos. It also makes it easier to manage the color schemes if they need to change.
317-374: Consider extracting location grouping logic into a helper function.The inline IIFE is clear but somewhat lengthy. Extracting this grouping logic into a small standalone function (e.g.,
groupBedAndNonBedLocations(results: LocationList[]) => { bedLocations; nonBedLocations }) could improve clarity and reusability.src/pages/Facility/locations/LocationNavbar.tsx (3)
1-31: Well-organized imports, but consider optimizing icon imports.The imports are well-structured with external dependencies first, followed by internal imports. However, you're importing 17 individual icons from 'lucide-react', which could impact bundle size.
Consider using a more selective approach:
-import { - Bed, - Building2, - ChevronDown, - ChevronRight, - Home, - Hotel, - LandPlot, - Layers, - LayoutGrid, - MapPin, - Route, - Split, - Store, - Truck, - Users, -} from "lucide-react"; +// Import only the icons actually used in the component +import { Bed, Building2, ChevronDown, ChevronRight, Home, Hotel } from "lucide-react"; +import { LandPlot, Layers, LayoutGrid, MapPin, Route } from "lucide-react"; +import { Split, Store, Truck, Users } from "lucide-react";Or consider using a dynamic import strategy if your bundler supports it.
191-193: Improve responsive design for the sidebar.The navigation sidebar is currently completely hidden on mobile screens (
hidden md:block). Consider implementing a collapsible/expandable mobile version instead of hiding it completely.- <div className="w-64 shadow-lg bg-white rounded-lg hidden md:block"> + <div className={cn( + "shadow-lg bg-white rounded-lg transition-all duration-300", + "w-64 md:w-64", // Control width based on screen size + "fixed md:static", // Fixed position on mobile, static on desktop + "top-0 bottom-0 left-0 z-50", // Position for mobile + isMobileMenuOpen ? "translate-x-0" : "-translate-x-full md:translate-x-0" // Slide in/out on mobile + )}> + <button + className="absolute -right-10 top-4 bg-white p-2 rounded-r-md shadow-md md:hidden" + onClick={() => setIsMobileMenuOpen(!isMobileMenuOpen)} + aria-label={isMobileMenuOpen ? t("close_menu") : t("open_menu")} + > + {isMobileMenuOpen ? <X className="h-5 w-5" /> : <Menu className="h-5 w-5" />} + </button> <div className="p-4">You would need to add state for
isMobileMenuOpenand import additional icons.
196-216: Consider adding pagination for large location lists.You're fetching up to 100 locations with a single request, but there's no handling for cases where there might be more than 100 locations. The ScrollArea component helps with displaying many items, but true pagination would be more efficient.
You could implement a "Load more" button that appears when there are more locations to load:
<ScrollArea> <div className="p-2"> {isLoadingLocations ? ( <div className="p-4"> <CardGridSkeleton count={3} /> </div> ) : ( + <> {topLevelLocations.map((location) => ( <LocationTreeNode key={location.id} location={location} selectedLocationId={selectedLocationId} onSelect={onLocationSelect} expandedLocations={expandedLocations} onToggleExpand={onToggleExpand} facilityId={facilityId} /> ))} + {allLocations?.next && ( + <Button + variant="outline" + className="w-full mt-2" + onClick={() => fetchNextPage()} + disabled={isFetchingNextPage} + > + {isFetchingNextPage ? t("loading") : t("load_more")} + </Button> + )} + </> )} </div> </ScrollArea>You would need to update the query to use
useInfiniteQueryinstead ofuseQuery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/Facility/locations/LocationContent.tsx(1 hunks)src/pages/Facility/locations/LocationList.tsx(1 hunks)src/pages/Facility/locations/LocationNavbar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Facility/locations/LocationList.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (1)
src/pages/Facility/locations/LocationContent.tsx (1)
62-66: Great fallback approach for unknown location forms.Returning a default icon if an unrecognized
formis passed ensures that the UI continues to function gracefully. This approach helps avoid errors when the codebase grows and more forms are introduced.
| const { data: children, isLoading } = useQuery({ | ||
| queryKey: [ | ||
| "locations", | ||
| facilityId, | ||
| "children", | ||
| selectedLocationId, | ||
| "kind", | ||
| "full", | ||
| currentPage, | ||
| searchQuery, | ||
| ], | ||
| queryFn: query(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| parent: selectedLocationId || undefined, | ||
| limit: ITEMS_PER_PAGE, | ||
| offset: (currentPage - 1) * ITEMS_PER_PAGE, | ||
| name: searchQuery || undefined, | ||
| ...(selectedLocationId ? {} : { mine: true, mode: "kind" }), | ||
| }, | ||
| }), | ||
| enabled: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for network or server failures.
Currently, the code checks only isLoading. If the query fails (isError), users might not see an explicit error message. Consider adding a fallback UI or toast notification to handle failures gracefully.
+ const { data: children, isLoading, isError } = useQuery({
...
+ onError: (error) => {
+ // Provide a user-friendly error message
+ console.error("Failed to fetch locations:", error);
+ },
});
+ if (isError) {
+ return (
+ <div className="p-6 text-center text-red-500">
+ {t("failed_to_load_locations")}
+ </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, isLoading } = useQuery({ | |
| queryKey: [ | |
| "locations", | |
| facilityId, | |
| "children", | |
| selectedLocationId, | |
| "kind", | |
| "full", | |
| currentPage, | |
| searchQuery, | |
| ], | |
| queryFn: query(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: selectedLocationId || undefined, | |
| limit: ITEMS_PER_PAGE, | |
| offset: (currentPage - 1) * ITEMS_PER_PAGE, | |
| name: searchQuery || undefined, | |
| ...(selectedLocationId ? {} : { mine: true, mode: "kind" }), | |
| }, | |
| }), | |
| enabled: true, | |
| }); | |
| const { data: children, isLoading, isError } = useQuery({ | |
| queryKey: [ | |
| "locations", | |
| facilityId, | |
| "children", | |
| selectedLocationId, | |
| "kind", | |
| "full", | |
| currentPage, | |
| searchQuery, | |
| ], | |
| queryFn: query(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| parent: selectedLocationId || undefined, | |
| limit: ITEMS_PER_PAGE, | |
| offset: (currentPage - 1) * ITEMS_PER_PAGE, | |
| name: searchQuery || undefined, | |
| ...(selectedLocationId ? {} : { mine: true, mode: "kind" }), | |
| }, | |
| }), | |
| enabled: true, | |
| onError: (error) => { | |
| // Provide a user-friendly error message | |
| console.error("Failed to fetch locations:", error); | |
| }, | |
| }); | |
| if (isError) { | |
| return ( | |
| <div className="p-6 text-center text-red-500"> | |
| {t("failed_to_load_locations")} | |
| </div> | |
| ); | |
| } |
| const { data: children, isLoading } = useQuery({ | ||
| queryKey: ["locations", facilityId, "children", location.id, "kind"], | ||
| queryFn: query.paginated(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| parent: location.id, | ||
| mode: "kind", | ||
| }, | ||
| pageSize: 100, | ||
| }), | ||
| enabled: isExpanded, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the location query.
The query for fetching child locations lacks error handling. If the API request fails, there's no feedback to the user.
Enhance the query implementation to handle errors:
- const { data: children, isLoading } = useQuery({
+ const { data: children, isLoading, error } = useQuery({
queryKey: ["locations", facilityId, "children", location.id, "kind"],
queryFn: query.paginated(locationApi.list, {
pathParams: { facility_id: facilityId },
queryParams: {
parent: location.id,
mode: "kind",
},
pageSize: 100,
}),
enabled: isExpanded,
});Then update the UI to display an error message if the request fails:
{isExpanded && error && (
<div className="pl-2 text-red-500 text-sm">
{t("failed_to_load_locations")}
</div>
)}| const { data: allLocations, isLoading: isLoadingLocations } = useQuery({ | ||
| queryKey: ["locations", facilityId, "mine", "kind"], | ||
| queryFn: query.paginated(locationApi.list, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| mine: true, | ||
| mode: "kind", | ||
| }, | ||
| pageSize: 100, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the main locations query.
Similar to the child locations query, the main query for all locations lacks error handling.
Enhance the query implementation to handle errors:
- const { data: allLocations, isLoading: isLoadingLocations } = useQuery({
+ const { data: allLocations, isLoading: isLoadingLocations, error: locationsError } = useQuery({
queryKey: ["locations", facilityId, "mine", "kind"],
queryFn: query.paginated(locationApi.list, {
pathParams: { facility_id: facilityId },
queryParams: {
mine: true,
mode: "kind",
},
pageSize: 100,
}),
});Then update the UI to display an error message if the request fails:
<ScrollArea>
<div className="p-2">
{isLoadingLocations ? (
<div className="p-4">
<CardGridSkeleton count={3} />
</div>
+ ) : locationsError ? (
+ <div className="p-4 text-red-500">
+ {t("failed_to_load_locations")}
+ </div>
) : (
topLevelLocations.map((location) => (
<LocationTreeNode📝 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: allLocations, isLoading: isLoadingLocations } = useQuery({ | |
| queryKey: ["locations", facilityId, "mine", "kind"], | |
| queryFn: query.paginated(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| mine: true, | |
| mode: "kind", | |
| }, | |
| pageSize: 100, | |
| }), | |
| }); | |
| const { data: allLocations, isLoading: isLoadingLocations, error: locationsError } = useQuery({ | |
| queryKey: ["locations", facilityId, "mine", "kind"], | |
| queryFn: query.paginated(locationApi.list, { | |
| pathParams: { facility_id: facilityId }, | |
| queryParams: { | |
| mine: true, | |
| mode: "kind", | |
| }, | |
| pageSize: 100, | |
| }), | |
| }); | |
| <ScrollArea> | |
| <div className="p-2"> | |
| {isLoadingLocations ? ( | |
| <div className="p-4"> | |
| <CardGridSkeleton count={3} /> | |
| </div> | |
| ) : locationsError ? ( | |
| <div className="p-4 text-red-500"> | |
| {t("failed_to_load_locations")} | |
| </div> | |
| ) : ( | |
| topLevelLocations.map((location) => ( | |
| <LocationTreeNode key={location.id} location={location} /> | |
| )) | |
| )} | |
| </div> | |
| </ScrollArea> |
| <div className="space-y-1"> | ||
| <div | ||
| className={cn( | ||
| "flex items-center py-1 px-2 rounded-md cursor-pointer hover:bg-gray-100", | ||
| isSelected && "bg-blue-100 text-blue-800", | ||
| )} | ||
| style={{ paddingLeft: `${level * 1}rem` }} | ||
| > | ||
| {hasChildren ? ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-6 w-6" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onToggleExpand(location.id); | ||
| }} | ||
| > | ||
| {isLoading ? ( | ||
| <div className="h-4 w-4 animate-spin rounded-full border-2 border-gray-300 border-t-gray-600" /> | ||
| ) : isExpanded ? ( | ||
| <ChevronDown className="h-4 w-4" /> | ||
| ) : ( | ||
| <ChevronRight className="h-4 w-4" /> | ||
| )} | ||
| </Button> | ||
| ) : ( | ||
| <span className="w-6" /> | ||
| )} | ||
| <div | ||
| className="flex items-center flex-1 text-sm gap-2" | ||
| onClick={() => onSelect(location)} | ||
| > | ||
| <Icon className="h-4 w-4" /> | ||
| <span className="truncate">{location.name}</span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and replace inline styles.
The current implementation has a few accessibility and styling issues:
- The clickable elements lack proper ARIA attributes
- Using inline styles for indentation is less maintainable
- The click handler is attached to a div instead of a button
Replace inline styles with CSS classes and improve accessibility:
<div className="space-y-1">
<div
className={cn(
- "flex items-center py-1 px-2 rounded-md cursor-pointer hover:bg-gray-100",
+ "flex items-center py-1 px-2 rounded-md cursor-pointer hover:bg-gray-100 location-node",
isSelected && "bg-blue-100 text-blue-800",
+ `pl-${level * 4}`
)}
- style={{ paddingLeft: `${level * 1}rem` }}
+ role="treeitem"
+ aria-expanded={hasChildren ? isExpanded : undefined}
+ aria-selected={isSelected}
>
{hasChildren ? (
<Button
variant="ghost"
size="icon"
className="h-6 w-6"
+ aria-label={isExpanded ? t("collapse") : t("expand")}
onClick={(e) => {
e.stopPropagation();
onToggleExpand(location.id);
}}
>
{isLoading ? (
<div className="h-4 w-4 animate-spin rounded-full border-2 border-gray-300 border-t-gray-600" />
) : isExpanded ? (
<ChevronDown className="h-4 w-4" />
) : (
<ChevronRight className="h-4 w-4" />
)}
</Button>
) : (
<span className="w-6" />
)}
- <div
- className="flex items-center flex-1 text-sm gap-2"
+ <button
+ type="button"
+ className="flex items-center flex-1 text-sm gap-2 bg-transparent border-none"
onClick={() => onSelect(location)}
+ aria-label={t("select_location", { name: location.name })}
>
<Icon className="h-4 w-4" />
<span className="truncate">{location.name}</span>
- </div>
+ </button>
</div>Then add the corresponding CSS classes to your stylesheet or use Tailwind's arbitrary values.
📝 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.
| <div className="space-y-1"> | |
| <div | |
| className={cn( | |
| "flex items-center py-1 px-2 rounded-md cursor-pointer hover:bg-gray-100", | |
| isSelected && "bg-blue-100 text-blue-800", | |
| )} | |
| style={{ paddingLeft: `${level * 1}rem` }} | |
| > | |
| {hasChildren ? ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-6 w-6" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onToggleExpand(location.id); | |
| }} | |
| > | |
| {isLoading ? ( | |
| <div className="h-4 w-4 animate-spin rounded-full border-2 border-gray-300 border-t-gray-600" /> | |
| ) : isExpanded ? ( | |
| <ChevronDown className="h-4 w-4" /> | |
| ) : ( | |
| <ChevronRight className="h-4 w-4" /> | |
| )} | |
| </Button> | |
| ) : ( | |
| <span className="w-6" /> | |
| )} | |
| <div | |
| className="flex items-center flex-1 text-sm gap-2" | |
| onClick={() => onSelect(location)} | |
| > | |
| <Icon className="h-4 w-4" /> | |
| <span className="truncate">{location.name}</span> | |
| </div> | |
| </div> | |
| <div className="space-y-1"> | |
| <div | |
| className={cn( | |
| "flex items-center py-1 px-2 rounded-md cursor-pointer hover:bg-gray-100 location-node", | |
| isSelected && "bg-blue-100 text-blue-800", | |
| `pl-${level * 4}` | |
| )} | |
| role="treeitem" | |
| aria-expanded={hasChildren ? isExpanded : undefined} | |
| aria-selected={isSelected} | |
| > | |
| {hasChildren ? ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-6 w-6" | |
| aria-label={isExpanded ? t("collapse") : t("expand")} | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onToggleExpand(location.id); | |
| }} | |
| > | |
| {isLoading ? ( | |
| <div className="h-4 w-4 animate-spin rounded-full border-2 border-gray-300 border-t-gray-600" /> | |
| ) : isExpanded ? ( | |
| <ChevronDown className="h-4 w-4" /> | |
| ) : ( | |
| <ChevronRight className="h-4 w-4" /> | |
| )} | |
| </Button> | |
| ) : ( | |
| <span className="w-6" /> | |
| )} | |
| <button | |
| type="button" | |
| className="flex items-center flex-1 text-sm gap-2 bg-transparent border-none" | |
| onClick={() => onSelect(location)} | |
| aria-label={t("select_location", { name: location.name })} | |
| > | |
| <Icon className="h-4 w-4" /> | |
| <span className="truncate">{location.name}</span> | |
| </button> | |
| </div> |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Nav Bar -> Encounters -> Locations Tab
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit