Fix: Add a new map view to the locations section of a facility settings #10821
Conversation
WalkthroughThis pull request introduces the React Flow library by adding a new dependency and corresponding CSS import, enabling interactive node-based UIs. A new location mapping component is created with dynamic node management and event handling. Enhancements were made to the location listing component, including improvements to facility data fetching, filtering logic using a Set for matched locations, and a new view-rendering function. In addition, a localization entry was added, and icon selection logic was refactored using a mapping object to simplify the codebase. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/types/location/location.ts (1)
75-91: Enhance type safety for your location-type icon mapping.
Currently,LocationTypeIconsis declared asRecord<string, LucideIcon>, allowing any string as a key. If possible, tighten the type to valid location form strings (e.g.,LocationFormOptions) to catch typos at compile time.src/pages/Facility/settings/locations/LocationMap.tsx (4)
35-38: Eliminate duplicated icon lookup logic.
ThisgetLocationTypeIconfunction duplicates some logic already inLocationCard. Consider centralizing icon lookups in a shared utility to avoid maintenance overhead.
185-185: Avoid relying on fixed delays in setTimeout.
A 100ms delay may occasionally be insufficient or excessive depending on rendering performance. Consider usingReactFlowevent callbacks or a more robust synchronization approach.
255-406: Refactor large nested layout logic for clarity.
TheuseEffectblock contains a complex recursive layout generation. Splitting it into smaller, well-named helper functions will improve readability and maintainability.
122-416: Consider extracting or modularizingLocationMapContent.
At over 200 lines, this component covers multiple responsibilities (layout, event handling, node expansions, etc.) Refactoring into smaller, focused components or utility functions will help keep it maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(1 hunks)public/locale/en.json(1 hunks)src/index.tsx(1 hunks)src/pages/Facility/settings/locations/LocationList.tsx(8 hunks)src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)src/types/location/location.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/index.tsx
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/types/location/location.ts
[error] 10-10: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (10)
package.json (1)
117-117: Nice addition of thereactflowdependency.
No immediate issues observed. Make sure it integrates well with your React environment and that the build process remains stable.src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
1-11: Streamlined imports and usage ofLocationTypeIcons.
This reduces repetition of icon imports throughout the code. Looks great!
23-24: Check for potential empty or undefinedformvalues.
Ifformcan ever be null or undefined, callingform.toLowerCase()will throw an error. Ensure you sanitize or defaultformto a valid string.src/pages/Facility/settings/locations/LocationMap.tsx (1)
70-71: Validate callback signature foronClick.
data.onClick({ id: data.id })might not match the full shape ofLocationListType. Confirm that the consuming code expects only{ id }or update the object to reflect the full location data.src/pages/Facility/settings/locations/LocationList.tsx (6)
1-1: Good addition of useQueryClient for cache management.Adding useQueryClient enables proper cache invalidation which is important for keeping the UI in sync with the backend data.
67-72: Well-implemented facility data query.The facility data query is properly structured using React Query patterns. This data is used to provide the facility name to the new map view component.
91-131: Excellent optimization of the filtering logic.The new filtering approach using a Set and two-pass algorithm is more efficient and maintainable:
- First pass identifies direct matches and their children
- Second pass adds parent chains for matched locations
This ensures the full hierarchy context is preserved while improving performance.
181-181: Good practice: Query invalidation after sheet closure.Invalidating the locations query after the sheet is closed ensures the UI displays fresh data after modifications, which is essential for a good user experience.
235-295: Improved code organization with extracted renderListView function.Extracting the list view rendering logic into a separate function improves code readability and maintainability. The JSX structure is well-organized with clear separation of concerns.
345-353: Clean implementation of conditional view rendering.The conditional rendering between list and map views is implemented cleanly. The LocationMap component receives all necessary props including locations, click handler, and facility name for context.
This implementation successfully fulfills the PR objective of adding a new map view to the locations section.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/pages/Facility/settings/locations/LocationMap.tsx (8)
29-33: Interface definition is clear and concise.The LocationMapProps interface provides a simple contract for the component with appropriate types. Consider adding JSDoc comments to document the purpose of each prop for better code documentation.
interface LocationMapProps { + /** Array of locations to be displayed in the map */ locations: LocationListType[]; + /** Callback function when a location node is clicked */ onLocationClick: (location: LocationListType) => void; + /** Name of the facility to be displayed as the root node */ facilityName: string; }
146-196: Consider optimizing the search effect implementation.The search effect is quite complex and includes commented code about expanding all nodes by default. The use of setTimeout for fitView is a workaround that might cause issues in certain scenarios.
Consider replacing the setTimeout with useLayoutEffect or a ref-based approach to ensure the view is updated after the state changes:
- useEffect(() => { + useLayoutEffect(() => { // ... existing code ... - setTimeout(() => { fitView({ padding: 0.2, minZoom: 0.1, maxZoom: 0.7, duration: 800, }); - }, 100); }, [locations, fitView, rootLocations]);Also, consider extracting the matchedIds logic into a separate useMemo hook for better code organization.
237-253: Consolidate node drag handlers to reduce code duplication.The onNodeDrag and onNodeDragStop functions share almost identical code, with the only difference being the fitView call in onNodeDragStop.
- const onNodeDrag = useCallback((_: ReactMouseEvent, node: Node) => { - setNodePositions((prev) => ({ - ...prev, - [node.id]: node.position, - })); - }, []); + const updateNodePosition = useCallback((node: Node, shouldFitView = false) => { + setNodePositions((prev) => ({ + ...prev, + [node.id]: node.position, + })); + if (shouldFitView) { + setTimeout(() => fitView({ padding: 0.2 }), 0); + } + }, [fitView]); + + const onNodeDrag = useCallback((_: ReactMouseEvent, node: Node) => { + updateNodePosition(node); + }, [updateNodePosition]); - const onNodeDragStop = useCallback( - (_: ReactMouseEvent, node: Node) => { - setNodePositions((prev) => ({ - ...prev, - [node.id]: node.position, - })); - setTimeout(() => fitView({ padding: 0.2 }), 0); - }, - [fitView], - ); + const onNodeDragStop = useCallback( + (_: ReactMouseEvent, node: Node) => { + updateNodePosition(node, true); + }, + [updateNodePosition], + );
255-273: Extract the width calculation logic into a separate function.The getWidth function is defined inside the useEffect, which means it gets recreated on every render cycle when any of the dependencies change. Consider moving it outside as a memoized function.
+ const getWidth = useCallback((location: LocationListType): number => { + if (!expandedNodes.includes(location.id)) return NODE_WIDTH; + + const children = locations.filter( + (loc) => loc.parent?.id === location.id, + ); + if (children.length === 0) return NODE_WIDTH; + + return Math.max( + NODE_WIDTH, + children.reduce((sum, child) => sum + getWidth(child), 0) + + (children.length - 1) * 40, + ); + }, [expandedNodes, locations]); useEffect(() => { const LEVEL_HEIGHT = 220; const NODE_WIDTH = 280; - const getWidth = (location: LocationListType): number => { - if (!expandedNodes.includes(location.id)) return NODE_WIDTH; - - const children = locations.filter( - (loc) => loc.parent?.id === location.id, - ); - if (children.length === 0) return NODE_WIDTH; - - return Math.max( - NODE_WIDTH, - children.reduce((sum, child) => sum + getWidth(child), 0) + - (children.length - 1) * 40, - ); - };
295-365: Break down the processLocation function for better maintainability.The processLocation function is quite complex and handles multiple responsibilities: node creation, edge creation, and child node positioning. Consider breaking it down into smaller, focused functions for better maintainability.
For example, you could extract the node creation and edge creation logic into separate functions:
const createLocationNode = (location, level, offsetX) => { const isExpanded = expandedNodes.includes(location.id); const childLocations = locations.filter( (loc) => loc.parent?.id === location.id, ); return { id: location.id, type: "custom", position: nodePositions[location.id] || { x: offsetX, y: level * LEVEL_HEIGHT, }, draggable: true, data: { name: location.name, type: t(`location_form__${location.form}`), form: location.form, childCount: childLocations.length, id: location.id, isExpanded, onToggle: toggleNode, onClick: (_loc: LocationListType) => onLocationClick(location), }, }; }; const createEdgeToParent = (location, parentX) => { if (parentX === null) return null; return { id: `${location.parent?.id}-${location.id}`, source: location.parent?.id || "", target: location.id, // ... rest of edge properties }; };
367-402: Add error handling for empty location arrays.The code assumes that rootLocations will contain at least one item. Consider adding explicit handling for cases where rootLocations is empty.
// Process filtered locations - if (rootLocations.length > 0) { + if (rootLocations.length > 0) { const totalRootWidth = rootLocations.reduce( (sum, loc) => sum + getWidth(loc), 0, ); // ... rest of the code + } else { + // Handle empty rootLocations case + // For example, show a message or create a default view + newNodes.push({ + id: "no-locations", + type: "custom", + position: { x: 0, y: LEVEL_HEIGHT }, + draggable: false, + data: { + name: t("no_locations_found"), + type: "", + form: "", + childCount: 0, + id: "no-locations", + isExpanded: false, + onToggle: () => {}, + onClick: () => {}, + }, + }); }
404-415: Optimize useEffect dependencies.The useEffect has many dependencies which could cause frequent re-renders. Consider using useCallback for functions like toggleNode and memoizing derived values to reduce the number of dependencies.
417-478: Well-structured ReactFlow implementation with appropriate controls.The ReactFlow implementation provides a good user experience with appropriate controls, panel options, and styling. Consider adding a reset button to allow users to reset the view if they drag nodes too far.
Add a reset view button to the panel:
<Panel position="top-right" className="bg-white/80 backdrop-blur-sm rounded-lg p-2 shadow-sm flex gap-2" > + <Button + variant="outline" + size="sm" + onClick={() => { + setNodePositions({}); + setTimeout(() => fitView({ padding: 0.2 }), 0); + }} + className="flex items-center gap-2" + title={t("reset_view")} + > + <RotateCcw className="h-4 w-4" /> + </Button> <Button variant="outline" size="sm" onClick={toggleAllNodes} className="flex items-center gap-2" >Don't forget to add the RotateCcw import:
-import { ChevronDown, ChevronRight, ChevronUp, Folder } from "lucide-react"; +import { ChevronDown, ChevronRight, ChevronUp, Folder, RotateCcw } from "lucide-react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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
🔇 Additional comments (5)
src/pages/Facility/settings/locations/LocationMap.tsx (5)
1-28: Good job with organizing imports and dependencies.The imports are well-structured with clear separation between external libraries, React hooks, and internal components. Using named imports helps with code readability and maintainability.
35-38: Well-implemented utility function for icon selection.The getLocationTypeIcon function elegantly handles icon selection with a fallback to the Folder icon when a specific icon isn't available.
40-116: CustomNode component is well-structured with good separation of concerns.The component handles click events correctly with stopPropagation to prevent bubbling and provides visual cues for expandable nodes. The styling with Tailwind classes makes the nodes visually appealing.
482-488: Clean component export with proper context provider.The LocationMap export component correctly wraps the LocationMapContent with ReactFlowProvider to provide the necessary context. This is a good pattern for ensuring that ReactFlow works correctly.
307-324:❓ Verification inconclusive
Add error handling or default values for node positioning.
The node position is determined by either the stored position in nodePositions or a calculated position. While there's a fallback with the OR operator, consider adding explicit error handling or default values for edge cases.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase where nodePositions is used without fallback values rg "nodePositions\[\w+\](?!\s*\|\|)" src/Length of output: 298
Action: Revisit Node Positioning Validity
The current code already provides a fallback using the OR operator:
position: nodePositions[location.id] || { x: offsetX, y: level * LEVEL_HEIGHT, },While this ensures that a default value is used when
nodePositions[location.id]is falsy, it doesn’t explicitly verify whether the retrieved value is valid or log/report potential issues. Please consider one or both of the following adjustments:
- Explicit Value Validation: Before using
nodePositions[location.id], validate that it is a well-formed position. For example, check that it contains valid numericalxandyvalues. If not, either apply the fallback or log an error.- Enhanced Error Handling: If neither the stored value nor the computed default produces a valid position, trigger an error or warning to aid debugging. This could include adding logging to report unexpected values.
These changes will help ensure that any edge cases regarding node positioning—such as misconfigured values—are caught and handled effectively. Please verify that this approach meets your requirements and that similar validations are applied wherever node positioning is used.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/types/location/location.ts (1)
1-11:⚠️ Potential issueFix shadow of the global
Mapproperty.The import statement imports
Mapfrom "lucide-react", which shadows the global JavaScriptMapobject. This was flagged by Biome static analysis.Apply this diff to rename the imported component:
import { Bed, Building, Building2, Car, Eye, Home, Hospital, LucideIcon, - Map, + Map as MapIcon, } from "lucide-react";🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🧹 Nitpick comments (1)
src/pages/Facility/settings/locations/LocationList.tsx (1)
195-232: Consider refactoring expansion logic to reuse filtering approach.The useEffect for expanding rows on search has similar logic to the new filtering approach but is implemented separately. Consider refactoring to reuse the Set-based approach for tracking matched locations.
You could potentially leverage the same
matchedLocationsSet from the filtering logic to determine which rows should be expanded, reducing code duplication and ensuring consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/Facility/settings/locations/LocationList.tsx(8 hunks)src/pages/Facility/settings/locations/components/LocationInfoCard.tsx(1 hunks)src/types/location/location.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/types/location/location.ts
[error] 10-10: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (6)
src/pages/Facility/settings/locations/components/LocationInfoCard.tsx (1)
6-48: LGTM: Clean implementation of the info card component.The component is well-structured with proper internationalization support through the
Transcomponent and responsive design with different content for desktop and mobile views.src/pages/Facility/settings/locations/LocationList.tsx (5)
66-71: Proper implementation of facility data fetching.Good use of React Query to fetch facility data with appropriate query key structure and path parameters.
93-101: Well-structured recursive helper function.The
addAllChildrenhelper function is concisely implemented and properly handles recursion to collect all descendants of a matched location.
103-130: Improved filtering logic with efficient Set implementation.The two-pass filtering algorithm significantly improves the previous implementation:
- First pass finds direct matches and their children
- Second pass adds parent chains for any matched location
This ensures all relevant locations in the hierarchy are included, improving user experience.
180-180: Good practice: Query invalidation after sheet close.Invalidating the locations query after closing the sheet ensures the UI displays fresh data, which is important for maintaining data consistency.
282-303: Well-structured conditional rendering for list/map views.The component cleanly separates the list and map views based on the active tab, allowing users to switch between different visualization methods.
I see the implementation includes the new
LocationInfoCardcomponent and theLocationMapcomponent with appropriate props, creating a cohesive user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/pages/Facility/settings/locations/LocationMap.tsx (6)
1-19: Be cautious about the large number of imports from ReactFlow.While the component uses many ReactFlow features, consider using the named import pattern for better code organization:
-import ReactFlow, { - Background, - ConnectionMode, - Controls, - Edge, - Handle, - MarkerType, - Node, - NodeProps, - Panel, - Position, - ReactFlowProvider, - useReactFlow, -} from "reactflow"; +import ReactFlow, { ReactFlowProvider, useReactFlow } from "reactflow"; +import { + Background, + ConnectionMode, + Controls, + Edge, + Handle, + MarkerType, + Node, + NodeProps, + Panel, + Position, +} from "reactflow";This approach can improve readability in large files.
41-117: Good implementation of the custom node component.The CustomNode component is well-structured with:
- Clear positioning of handles
- Visual indicators for parent nodes
- Proper event handling
- Consistent styling
One suggestion to improve accessibility:
<Button variant="ghost" size="sm" className="h-8 px-2 hover:bg-gray-100 transition-colors" onClick={handleToggle} + aria-expanded={data.isExpanded} + aria-label={data.isExpanded ? t("collapse_children") : t("expand_children")} >
147-197: Consider extracting the search and match logic to a separate hook or utility function.The location search processing logic is quite complex and defined within a useEffect, making it harder to test and reuse. Consider extracting this to a custom hook like
useLocationMatchingor a utility function to improve maintainability.Also, the hard-coded timeout for fitView might be problematic if node rendering takes longer than expected.
185-186: Remove commented code before production.Commented code like this should be removed before merging to production unless it's meant to be documentation. If it's for development purposes, consider using a feature flag instead.
199-219: Implement throttling for the fit view operations.Multiple calls to
fitViewwithin a short time period (especially during user interactions) could cause performance issues and visual jitter. Consider implementing throttling or debouncing for these operations.// Using lodash throttle import { throttle } from 'lodash'; // Create a throttled version of fitView const throttledFitView = useCallback( throttle((options) => { fitView(options); }, 300), [fitView] );
418-479: Consider adding error boundaries for ReactFlow.Since ReactFlow is a complex library that renders an interactive visualization, it's good practice to wrap it in an error boundary to prevent the entire application from crashing if there's an issue with the visualization.
import { ErrorBoundary } from 'react-error-boundary'; // Inside your component <ErrorBoundary fallback={<div className="p-4 bg-red-50 text-red-700 rounded">Failed to load location map</div>} > <ReactFlow nodes={nodes} edges={edges} // other props > {/* children */} </ReactFlow> </ErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Facility/settings/locations/components/LocationCard.tsx (2)
11-15: Great refactoring approach!The switch to using a mapped object
LocationTypeIconsfor determining icons is a cleaner, more maintainable approach compared to a lengthy switch statement. This change reduces code duplication and makes it easier to modify icon associations in the future.
26-28: Clean implementation of the icon selection logic.The refactored function is concise and effective, with proper fallback to the
Foldericon when no matching icon is found for a given location form.src/pages/Facility/settings/locations/LocationMap.tsx (2)
483-489: Good separation of concerns with ReactFlowProvider.Separating the provider from the implementation makes the component more maintainable and testable. This is a good practice when working with context-based libraries like ReactFlow.
1-490: Add comprehensive testing for this complex component.This location map component has complex state management and interactions. Given its importance in the facility settings, please ensure thorough testing with:
- Unit tests for the utility functions and custom hooks
- Component tests for interactions like expanding/collapsing nodes
- Integration tests with the location data flow
This will help catch regressions when future changes are made.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/pages/Facility/settings/locations/LocationMap.tsx (6)
41-44: Add coverage for fallback icon logic.Currently, the code uses a fallback (
Folder) if theformtype is not recognized. It might be useful to add a unit test or snapshot test to confirm the correct fallback icon is rendered.
96-117: Enhance accessibility for toggle button.Consider adding descriptive ARIA labels or an accessible name so screen readers can announce the purpose of the toggle control (e.g., “Expand node children”).
139-147: Consider memoization to address potential performance bottlenecks.Repeatedly calling
calculateNodeWidthfor each node and filter/reduce on large data sets may become expensive. Memoizing results keyed by(location.id, expandedNodes)or caching sub-tree calculations can help reduce overhead.
228-298: Consider further splitting large recursive logic to improve maintainability.While the location hierarchy is processed recursively, the
processLocationHierarchyfunction is relatively large. Breaking it into smaller helper functions for node/edge creation, child calculation, and position computation could further improve clarity and testability.
398-413: Clear timeouts when unmounting to avoid potential memory leaks.The
toggleAllNodesfunction schedules a delayedfitViewusingsetTimeout. If the component is unmounted before the timer fires, React cannot clean it up automatically. Consider storing and clearing the timeout to prevent such leaks.
566-572: Add tests for new map functionality.This PR adds a major feature without showing any test coverage for edge cases (e.g., deeply nested nodes, empty location sets, or toggling states). Tests would help ensure correctness and reduce regressions.
Would you like assistance setting up a test suite for this component?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/settings/locations/LocationMap.tsx (1)
434-490: Verify behavior with empty or missing location data.When
rootLocationsorlocationsis empty, it's worth checking if the code gracefully handles the absence of nodes and edges. Ensure that no unexpected errors occur (e.g., attempting to create edges from undefined data).
| default: | ||
| return <Folder className="h-5 w-5" />; | ||
| } | ||
| const getLocationTypeIcon = (form: LocationForm) => { |
There was a problem hiding this comment.
move to location.ts since it's used in multiple files
There was a problem hiding this comment.
@abhimanyurajeesh i'd say do #10821 (review) here instead
There was a problem hiding this comment.
Thank you, will try that
Just focus on the node that is being expanded or did you mean the child nodes of the parent? |
@Jacobjeevan did you mean like this? Screen.Recording.2025-03-04.at.9.01.33.AM.mov |
Somewhat 🤔 On expansion, it still zooms out for a second there (0:08 onwards..instead of just zooming in directly to focus on the node that's expanded). On collapse, focus on node's (that's being collapsed) parent. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/pages/Facility/settings/locations/LocationMap.tsx (7)
29-33: Consider using CSS variables for layout constants.These constants could be defined as CSS variables to make them more maintainable and easier to adjust for responsive design needs.
-// Constants -const LEVEL_HEIGHT = 220; -const NODE_WIDTH = 280; -const ROOT_SPACING = 100; +// Constants - consider moving these to CSS variables in a future refactor +const LEVEL_HEIGHT = 220; +const NODE_WIDTH = 280; +const ROOT_SPACING = 100;
44-45: Type refinement improvement.The form type could be explicitly typed to avoid the type assertion.
- const Icon = - LocationTypeIcons[data.form as keyof typeof LocationTypeIcons] || Folder; + const Icon = LocationTypeIcons[data.form] || Folder;
361-365: Use optional chaining for more concise code.The static analysis correctly identifies an opportunity for optional chaining.
- if (searchQuery && searchQuery.trim()) { + if (searchQuery?.trim()) { setExpandedNodes(Array.from(matchedIds)); } else { setExpandedNodes([]); // Collapse all nodes when search is cleared }🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
460-476: Optimize node drag performance with debouncing.The current implementation updates node positions on every drag event, which could impact performance with many nodes. Consider debouncing the position updates.
+import { debounce } from "lodash"; // Add import at the top // Replace onNodeDrag and onNodeDragStop with: +const updateNodePosition = useCallback( + (nodeId: string, position: { x: number; y: number }) => { + setNodePositions((prev) => ({ + ...prev, + [nodeId]: position, + })); + }, + [] +); + +const debouncedFitView = useCallback( + debounce(() => { + fitView({ padding: 0.2 }); + }, 100), + [fitView] +); + +const onNodeDrag = useCallback((_: ReactMouseEvent, node: Node) => { + updateNodePosition(node.id, node.position); +}, [updateNodePosition]); + +const onNodeDragStop = useCallback( + (_: ReactMouseEvent, node: Node) => { + updateNodePosition(node.id, node.position); + debouncedFitView(); + }, + [updateNodePosition, debouncedFitView] +);
546-608: Implement zooming constraints for better user experience.When users interact with the map, especially on complex location hierarchies, they might zoom too far in or out. Consider adding constraints or smart zoom defaults.
Add more specific zoom constraints to prevent users from getting lost in the visualization:
fitView={true} fitViewOptions={{ padding: 0.2, minZoom: 0.2, maxZoom: 0.8, + includeHiddenNodes: false, duration: 800, }} - minZoom={0.1} - maxZoom={2} + minZoom={0.15} + maxZoom={1.5} + zoomOnScroll={true} + zoomOnPinch={true} + panOnScroll={true} + panOnScrollMode="free" connectionMode={ConnectionMode.Loose}
34-39: Add JSDoc comments to the component props interface.Adding proper JSDoc documentation to the interface would improve maintainability and developer experience.
+/** + * Props for the LocationMap component + * @property {LocationListType[]} locations - Array of location objects to display + * @property {Function} onLocationClick - Handler for when a location is clicked + * @property {string} facilityName - Name of the facility to display as root + * @property {string} [searchQuery] - Optional search query to filter locations + */ interface LocationMapProps { locations: LocationListType[]; onLocationClick: (location: LocationListType) => void; facilityName: string; searchQuery?: string; }
329-340: Memoize recursive functions for better performance.The recursive
processLocationfunction is created on every render. Consider memoizing it using useCallback.+ // Memoize the processLocation function + const processLocation = useCallback((location: LocationListType) => { + matchedIds.add(location.id); + + // Find and process all children + const children = locations.filter( + (loc) => loc.parent?.id === location.id, + ); + children.forEach((child) => { + processLocation(child); // Recursively process each child + }); + }, [locations, matchedIds]); - // Add all locations and their children to matched IDs - const processLocation = (location: LocationListType) => { - matchedIds.add(location.id); - - // Find and process all children - const children = locations.filter( - (loc) => loc.parent?.id === location.id, - ); - children.forEach((child) => { - processLocation(child); // Recursively process each child - }); - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/Facility/settings/locations/LocationList.tsx(8 hunks)src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)src/pages/Facility/settings/locations/components/LocationCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Facility/settings/locations/LocationList.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/locations/LocationMap.tsx
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/pages/Facility/settings/locations/components/LocationCard.tsx (3)
1-1: Improved import efficiency with icon mapping.The refactoring to use
LocationTypeIconsfrom a centralized location is an excellent change that addresses the previous review comments. This approach is much more maintainable than importing individual icons and using a switch statement.Also applies to: 11-11
21-23: Clean implementation of icon selection with fallback.This is a well-implemented approach for dynamic icon selection:
- Uses type safety with the keyof typeof assertion
- Provides a sensible Folder fallback when the form type doesn't match
- Makes the component more maintainable and less prone to errors
31-31: Consistent icon usage.The Icon component is properly used here, maintaining consistency with the PR's design patterns.
src/pages/Facility/settings/locations/LocationMap.tsx (3)
75-75: Consider using relative width for better responsiveness.As suggested in previous comments, fixed width values can lead to layout issues on different screen sizes. While I understand your point that relative widths might look odd in certain cases, consider a responsive approach using CSS utility classes or media queries.
387-435:❓ Verification inconclusive
Address focus behavior when expanding/collapsing nodes.
Per the PR comments, the current behavior zooms out before focusing on expanded nodes, which isn't ideal. Consider improving this by:
- When expanding a node, immediately focus on that node and its children
- When collapsing a node, focus on the parent node
You might want to adjust the fitView timing and parameters to create a smoother transition.
This implementation could be revised to address the zoom-out-before-zoom-in issue mentioned in PR comments. Try experimenting with direct fitView calls without setTimeout, or different duration/padding values to create a smoother transition.
Action Required: Refine Node Focus Transition
For Expansion:
Instead of delaying the focus withsetTimeout, invokefitViewimmediately so that the expanded node and its immediate children are focused without an initial zoom-out effect.For Collapse:
Change the approach from focusing on the entire visible tree to focusing on the parent node. This means adjusting or replacing the logic ingetAllVisibleNodesto target just the parent, ensuring a smoother transition when collapsing nodes.Please adjust the timing and parameters (e.g., duration, padding, minZoom/maxZoom) of the
fitViewcalls based on the new focus behavior, and test the transition for a seamless user experience.
4-18:✅ Verification successful
Ensure ReactFlow styles are properly imported in the project.
The ReactFlow component requires its styles to be imported. Make sure these styles are imported somewhere in your application (typically in a main CSS file or index file).
🏁 Script executed:
#!/bin/bash # Check if ReactFlow styles are imported in the project rg -l "import 'reactflow/dist/style.css'" --type ts --type tsx rg -l "import \"reactflow/dist/style.css\"" --type ts --type tsxLength of output: 191
The previous script encountered an error because the file type "tsx" wasn’t recognized. Please run this revised command to verify whether the ReactFlow styles import (i.e. "reactflow/dist/style.css") exists in your project. Once confirmed manually, you can ensure that the styles are properly imported (typically in a global styles file or in the main index file).
🏁 Script executed:
#!/bin/bash # Revised search: Look for the ReactFlow styles import across TS, TSX, JS, and JSX files. rg -l "reactflow/dist/style.css" -g "*.{ts,tsx,js,jsx}"Length of output: 191
ReactFlow styles are correctly imported – no changes required.
The search confirmed that the ReactFlow styles (i.e., the import of
"reactflow/dist/style.css") exist in the project, specifically insrc/index.tsx. This ensures that the styles needed by the ReactFlow component are properly loaded. Please keep this global import intact to continue proper styling.
Oh ya got it. |
Screen.Recording.2025-03-04.at.4.34.00.PM.mov@Jacobjeevan can you please check this out? I hope this will do 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/Facility/settings/locations/LocationMap.tsx (1)
368-375: 🛠️ Refactor suggestionReplace setTimeout with useLayoutEffect for view fitting.
Using setTimeout for layout operations is not ideal and can cause flashing or janky animations.
- // Fit view after nodes are updated - setTimeout(() => { - fitView({ - padding: 0.2, - minZoom: 0.2, - maxZoom: 0.7, - duration: 800, - }); - }, 100); + // Use a layout effect to fit view after DOM updates + const timeoutRef = useRef<NodeJS.Timeout>(); + + useLayoutEffect(() => { + timeoutRef.current = setTimeout(() => { + fitView({ + padding: 0.2, + minZoom: 0.2, + maxZoom: 0.7, + duration: 800, + }); + }, 100); + + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, [fitView, expandedNodes]);Don't forget to add useRef and useLayoutEffect to your imports:
import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react";
🧹 Nitpick comments (7)
src/pages/Facility/settings/locations/LocationMap.tsx (7)
67-71: Consider using dynamic styling for node background elements.The UI elements for representing nodes with children use fixed styling with absolute positioning and hard-coded values. This could lead to inconsistent appearance if the node dimensions change.
- <> - <div className="absolute -bottom-2 left-1/2 -translate-x-1/2 w-[93%] h-full border-2 border-gray-200 rounded-lg bg-white" /> - <div className="absolute -bottom-1 left-1/2 -translate-x-1/2 w-[97%] h-full border-2 border-gray-200 rounded-lg bg-white" /> - </> + <> + <div className="absolute -bottom-2 left-1/2 -translate-x-1/2 w-[calc(100%-20px)] h-full border-2 border-gray-200 rounded-lg bg-white" /> + <div className="absolute -bottom-1 left-1/2 -translate-x-1/2 w-[calc(100%-10px)] h-full border-2 border-gray-200 rounded-lg bg-white" /> + </>
105-105: Enhance the i18n string for better pluralization.The current translation string doesn't support pluralization, which may lead to grammatically incorrect messages in some languages when the child count is not 1.
- {data.childCount} {t("level_inside")} + {data.childCount} {t("level_inside", { count: data.childCount })}Also, ensure your i18n configuration supports pluralization rules for all required languages.
329-340: Use a more efficient algorithm to process location hierarchies.The current recursive approach to process locations and their children is not optimized for performance with large hierarchies.
Consider using a more efficient approach to traverse the location tree:
- // Add all locations and their children to matched IDs - const processLocation = (location: LocationListType) => { - matchedIds.add(location.id); - - // Find and process all children - const children = locations.filter( - (loc) => loc.parent?.id === location.id, - ); - children.forEach((child) => { - processLocation(child); // Recursively process each child - }); - }; + // Build a map of parent IDs to children for faster lookup + const childrenMap = locations.reduce((map, loc) => { + if (loc.parent?.id) { + if (!map[loc.parent.id]) map[loc.parent.id] = []; + map[loc.parent.id].push(loc); + } + return map; + }, {} as Record<string, LocationListType[]>); + + // Process locations using a queue to avoid deep recursion + const processLocation = (location: LocationListType) => { + const queue = [location]; + while (queue.length > 0) { + const current = queue.shift()!; + matchedIds.add(current.id); + + // Add children to the queue + const children = childrenMap[current.id] || []; + queue.push(...children); + } + };
361-365: Optimize search query handling with selective expansion.Currently, when a search query is present, all nodes are expanded, which can lead to performance issues with large hierarchies.
Only expand nodes that are relevant to the search:
// Expand all nodes when there's a search query, collapse all when search is cleared if (searchQuery && searchQuery.trim()) { - setExpandedNodes(Array.from(matchedIds)); + // Get all parent IDs to ensure they're expanded + const parentsToExpand = new Set<string>(); + locations.forEach(loc => { + if (matchedIds.has(loc.id) && loc.parent?.id) { + let current = loc; + while (current.parent?.id) { + parentsToExpand.add(current.parent.id); + current = locations.find(l => l.id === current.parent?.id) || current; + if (current.id === current.parent?.id) break; // Prevent infinite loop + } + } + }); + + setExpandedNodes(Array.from(parentsToExpand)); } else { setExpandedNodes([]); // Collapse all nodes when search is cleared }🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
428-436: Apply the same useLayoutEffect pattern for toggle operations.Similar to the previous setTimeout issue, this toggleAllNodes function has the same potential for flashing UI issues.
Follow the same pattern as recommended for the search query effect, replacing setTimeout with a properly managed effect.
535-535: Add aria attributes for better accessibility.The map container lacks necessary ARIA attributes for accessibility.
- <div className="h-[600px] w-full bg-gray-50 rounded-lg border"> + <div + className="h-[600px] w-full bg-gray-50 rounded-lg border" + role="region" + aria-label={t("location_map")} + >
570-571: Consider user preference for reduced motion.The current implementation doesn't account for users who prefer reduced motion.
Add a check for the prefers-reduced-motion media query:
import { useCallback, useEffect, useMemo, useState } from "react"; +import { usePrefersReducedMotion } from "@/hooks/usePrefersReducedMotion"; // Later in the component: + const prefersReducedMotion = usePrefersReducedMotion(); // And when configuring animations: zoomOnScroll={true} zoomOnPinch={true} panOnScroll={true} panOnDrag={true} + animateElements={!prefersReducedMotion} connectionMode={ConnectionMode.Loose}You'll need to create a hook like this if you don't have one:
// hooks/usePrefersReducedMotion.ts import { useEffect, useState } from 'react'; export function usePrefersReducedMotion() { const [prefersReducedMotion, setPrefersReducedMotion] = useState(false); useEffect(() => { const mediaQuery = window.matchMedia('(prefers-reduced-motion: reduce)'); setPrefersReducedMotion(mediaQuery.matches); const handleChange = () => { setPrefersReducedMotion(mediaQuery.matches); }; mediaQuery.addEventListener('change', handleChange); return () => { mediaQuery.removeEventListener('change', handleChange); }; }, []); return prefersReducedMotion; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/locations/LocationMap.tsx
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/locations/LocationMap.tsx (3)
44-45: Use object mapping pattern consistently.The Icon assignment could be simplified by using object access pattern with the LocationTypeIcons map. This avoids unnecessary conditional logic.
- const Icon = - LocationTypeIcons[data.form as keyof typeof LocationTypeIcons] || Folder; + const Icon = LocationTypeIcons[data.form] || Folder;
74-76: Consider using relative width for node content.The node content has a fixed width of 240px, which might not be responsive to different screen sizes or user preferences.
While fixed width is acceptable for this type of visualization, consider using relative width units with max-width constraints for better responsive behavior:
- "relative w-[240px] bg-white rounded-lg border-2 overflow-hidden shadow-sm cursor-pointer", + "relative w-full max-w-[240px] bg-white rounded-lg border-2 overflow-hidden shadow-sm cursor-pointer",
1-609: Great job implementing the location map feature!This is a well-structured implementation of the ReactFlow-based location map. The code is organized into small, reusable functions with clear responsibilities, and it handles the complex task of hierarchical node layout effectively. The component delivers on all the requirements mentioned in the PR objectives:
- ✅ Hierarchical Display - shows parent-child relationships
- ✅ Interactive Navigation - supports panning, zooming, and focusing
- ✅ Dynamic Search - implemented search with automatic expansion
- ✅ Expandable/Collapsible Nodes - toggle functionality for each node
- ✅ Drag and Drop Support - nodes can be moved around
- ✅ Auto-fit View - automatically adjusts the view to show relevant nodes
The PR effectively implements the new PHC Location Map/List feature for testing and demonstration purposes as requested.
🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Great 👍
Well, not a big deal per say, depends on what we want product side. |
I guess for now, I’ll quickly hide the chevron for the root. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/pages/Facility/settings/locations/LocationMap.tsx (6)
44-45: Apply the enhanced icon selection pattern.The current icon selection logic can be simplified using object access with fallback. This makes the code more concise and follows the pattern recommended in previous feedback.
- const Icon = - LocationTypeIcons[data.form as keyof typeof LocationTypeIcons] || Folder; + const Icon = LocationTypeIcons[data.form] || Folder;
68-71: Consider refactoring the UI stack effect for better performance.Creating absolute positioned elements for the stacked card effect may impact performance for large trees. Consider using box-shadow or a simpler approach for the visual stack effect.
- <> - <div className="absolute -bottom-2 left-1/2 -translate-x-1/2 w-[93%] h-full border-2 border-gray-200 rounded-lg bg-white" /> - <div className="absolute -bottom-1 left-1/2 -translate-x-1/2 w-[97%] h-full border-2 border-gray-200 rounded-lg bg-white" /> - </> + <div + className="absolute -bottom-2 left-1/2 -translate-x-1/2 w-[97%] h-full rounded-lg bg-white" + style={{ + boxShadow: '0 0 0 2px #e5e7eb, 0 -2px 0 3px #f3f4f6' + }} + />
75-75: Consider using relative width instead of fixed width.Using a fixed width of "w-[240px]" may cause layout issues on different screen sizes. Consider using relative width for better responsiveness.
I notice this was previously commented on and your response indicated changing it might look odd. If you've tested this and confirmed fixed width is better for your specific application, then this is fine to keep as is.
362-362: Use optional chaining for safer search query handling.The static analysis tool suggests using optional chaining when checking searchQuery, which is a good practice for handling potentially undefined values.
- if (searchQuery && searchQuery.trim()) { + if (searchQuery?.trim()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 362-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
421-440: Consistent use of setTimeout instead of requestAnimationFrame.In toggleAllNodes, you're using setTimeout while in toggleNode you're using requestAnimationFrame. For consistency, consider using requestAnimationFrame in both places.
const toggleAllNodes = useCallback(() => { setNodePositions({}); setExpandedNodes((prev) => { const isExpanding = prev.length !== locations.length; const newExpandedNodes = isExpanding ? locations.map((loc) => loc.id) : []; - setTimeout(() => { + requestAnimationFrame(() => { fitView({ padding: 0.2, minZoom: 0.1, maxZoom: 0.8, duration: 800, }); - }, 100); + }); return newExpandedNodes; }); }, [locations, fitView]);
535-601: Consider extracting panel controls to a separate component.The Panel component with the expand/collapse button could be extracted to improve readability and maintainability of the main component.
You could create a separate component like:
const MapControls = ({ expandedNodes, locations, toggleAllNodes }: { expandedNodes: string[], locations: LocationListType[], toggleAllNodes: () => void }) => { const { t } = useTranslation(); return ( <Panel position="top-right" className="bg-white/80 backdrop-blur-sm rounded-lg p-2 shadow-sm" > <Button variant="outline" size="sm" onClick={toggleAllNodes} className="flex items-center gap-2" > {expandedNodes.length === locations.length ? ( <> <ChevronUp className="h-4 w-4" /> {t("collapse_all")} </> ) : ( <> <ChevronDown className="h-4 w-4" /> {t("expand_all")} </> )} </Button> </Panel> ); };Then use it within your ReactFlow component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/locations/LocationMap.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/locations/LocationMap.tsx
[error] 362-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (6)
src/pages/Facility/settings/locations/LocationMap.tsx (6)
107-113: Good implementation of hiding chevrons for the root node.The conditional check to ensure chevrons are not shown for facility nodes is a good implementation following previous feedback.
348-359: Optimize parent lookup with a parent map.The current logic to find and include parents is inefficient, as it repeatedly searches the locations array.
This is a duplicate of a previously identified issue that was mentioned in past review comments. Please consider implementing the suggested parent map optimization to improve performance.
386-413: Good implementation of focused view on node toggle.The new focus behavior when expanding/collapsing nodes directly addresses the PR comments about view behavior. The code now properly focuses on the relevant node and its children when expanding, and on the parent node when collapsing.
1-33: Comprehensive import and constant setup.The imports are well-organized and the constants for layout parameters are properly defined at the top of the file, making it easier to adjust layout properties.
34-39: Well-defined interface for component props.The LocationMapProps interface is clear and properly typed, providing a good contract for component usage.
604-610: Good use of ReactFlowProvider wrapping.Properly wrapping the main component with ReactFlowProvider ensures that all ReactFlow context is available to child components.
|
Issue need to taken as a separate PR
Product Enhancement (Post User requirement after understanding scope)
|
|
@abhimanyurajeesh Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Key Features
Screen.Recording.2025-02-26.at.3.00.15.PM.mov
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Refactor