[Netmanager] Grids and Cohorts pages & functionality #2418
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several new React components across the netmanager-app, focusing on cohorts, grids, and associated management interfaces. The changes include detailed pages for cohorts and grids, dialog components for creating and assigning devices, and utility components like a date range picker and textarea. Additionally, updates to existing hooks and state management structures enhance the application's user interface for managing network infrastructure, providing more granular control and interaction capabilities. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (13)
netmanager-app/components/ui/textarea.tsx (1)
5-19: LGTM! Well-structured textarea component.The implementation follows React best practices with proper TypeScript typing and accessibility features.
Consider adding JSDoc comments to document the component's props and usage:
+/** + * A styled textarea component that supports all standard HTML textarea attributes. + * @component + * @example + * <Textarea + * placeholder="Enter description..." + * className="custom-class" + * /> + */ const Textarea = React.forwardRef<netmanager-app/components/grids/date-range-picker.tsx (1)
16-22: Consider making the initial date range configurable.The component has a hardcoded initial date range of 7 days. This should be configurable through props for better reusability.
export function DateRangePicker({ className, + initialDateRange = { + from: new Date(), + to: addDays(new Date(), 7), + }, + onDateRangeChange?: (range: DateRange | undefined) => void, }: React.HTMLAttributes<HTMLDivElement> & { + initialDateRange?: DateRange; + onDateRangeChange?: (range: DateRange | undefined) => void; }) { - const [date, setDate] = React.useState<DateRange | undefined>({ - from: new Date(), - to: addDays(new Date(), 7), - }); + const [date, setDate] = React.useState<DateRange | undefined>(initialDateRange);netmanager-app/components/grids/grid-sites.tsx (2)
53-57: Optimize search implementation.The current search implementation could be optimized by:
- Debouncing the search input
- Memoizing the filtered results
- Adding type-ahead functionality
+import { useMemo } from 'react'; +import { useDebounce } from '@/hooks/use-debounce'; export function GridSitesTable() { const [searchQuery, setSearchQuery] = useState(""); + const debouncedSearch = useDebounce(searchQuery, 300); - const filteredSites = sites.filter((site) => + const filteredSites = useMemo(() => sites.filter((site) => Object.values(site).some((value) => - value.toLowerCase().includes(searchQuery.toLowerCase()) + value.toLowerCase().includes(debouncedSearch.toLowerCase()) ) - ); + ), [debouncedSearch]);
71-73: Implement download functionality.The download button is currently non-functional. Consider implementing CSV export functionality.
Would you like me to provide an implementation for the CSV export functionality?
netmanager-app/app/(authenticated)/cohorts/page.tsx (2)
60-70: Move date formatting to a utility function.The
formatDatefunction should be moved to a shared utility file as it could be reused across components.+// utils/date.ts +export const formatDate = (dateString: string) => { + return new Date(dateString).toLocaleString("en-US", { + year: "numeric", + month: "2-digit", + day: "2-digit", + hour: "2-digit", + minute: "2-digit", + second: "2-digit", + hour12: false, + }); +};
108-112: Add loading state for navigation.Consider adding a loading state when navigating to cohort details.
+import { useState } from "react"; +import { Loader2 } from "lucide-react"; export default function CohortsPage() { + const [loadingCohort, setLoadingCohort] = useState<string | null>(null); + + const handleCohortClick = async (cohortName: string) => { + setLoadingCohort(cohortName); + await router.push(`/cohorts/${cohortName}`); + setLoadingCohort(null); + }; return ( <TableRow key={cohort.name} className="cursor-pointer" - onClick={() => router.push(`/cohorts/${cohort.name}`)} + onClick={() => handleCohortClick(cohort.name)} > + {loadingCohort === cohort.name && ( + <Loader2 className="h-4 w-4 animate-spin" /> + )}netmanager-app/components/grids/create-grid.tsx (1)
114-118: Consider improving the shapefile input UX.The current implementation uses a textarea for shapefile data, which might be error-prone. Consider implementing a visual polygon drawing interface on the map.
netmanager-app/components/cohorts/assign-cohort-devices.tsx (1)
102-105: Consider adding loading and error states.The device search interface lacks loading and error states when fetching devices.
<CommandInput placeholder="Search devices..." /> <CommandList> - <CommandEmpty>No devices found.</CommandEmpty> + <CommandEmpty> + {isLoading ? ( + <div className="p-4 text-center">Loading devices...</div> + ) : error ? ( + <div className="p-4 text-center text-red-500">Failed to load devices</div> + ) : ( + "No devices found." + )} + </CommandEmpty>netmanager-app/components/cohorts/create-cohort.tsx (2)
67-69: Hardcoded network value limits flexibility.The network field is disabled with a hardcoded "airqo" value. Consider making this field dynamic if multiple networks need to be supported in the future.
- network: "airqo", + network: "", // or fetch from user context/config
62-62: Remove commented code.The commented-out useState declaration should be removed if it's not being used.
- // const [selectedDevices, setSelectedDevices] = useState<string[]>([])netmanager-app/app/(authenticated)/grids/page.tsx (1)
74-84: Consider using a date formatting library.The date formatting logic could be simplified using a library like
date-fnsordayjs.+import { format } from 'date-fns'; -const formatDate = (dateString: string) => { - return new Date(dateString).toLocaleString("en-US", { - year: "numeric", - month: "2-digit", - day: "2-digit", - hour: "2-digit", - minute: "2-digit", - second: "2-digit", - hour12: false, - }); -}; +const formatDate = (dateString: string) => { + return format(new Date(dateString), 'yyyy-MM-dd HH:mm:ss'); +};netmanager-app/app/(authenticated)/grids/[id]/page.tsx (1)
186-186: Add error boundary for GridSitesTable component.The
GridSitesTablecomponent is rendered without error handling, which could lead to the entire page crashing if the component fails.+import { ErrorBoundary } from '@/components/error-boundary'; + -<GridSitesTable /> +<ErrorBoundary fallback={<div>Error loading grid sites table</div>}> + <GridSitesTable /> +</ErrorBoundary>netmanager-app/app/(authenticated)/cohorts/[id]/page.tsx (1)
82-92: Use a date formatting library for consistent date handling.The custom date formatting function could be replaced with a library like
date-fnsfor more consistent and locale-aware date formatting.+import { format } from 'date-fns'; + - const formatDate = (dateString: string) => { - return new Date(dateString).toLocaleString("en-US", { - year: "numeric", - month: "2-digit", - day: "2-digit", - hour: "2-digit", - minute: "2-digit", - second: "2-digit", - hour12: false, - }); - }; + const formatDate = (dateString: string) => { + return format(new Date(dateString), 'yyyy-MM-dd HH:mm:ss'); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
netmanager-app/app/(authenticated)/cohorts/[id]/page.tsx(1 hunks)netmanager-app/app/(authenticated)/cohorts/page.tsx(1 hunks)netmanager-app/app/(authenticated)/grids/[id]/page.tsx(1 hunks)netmanager-app/app/(authenticated)/grids/page.tsx(1 hunks)netmanager-app/components/cohorts/assign-cohort-devices.tsx(1 hunks)netmanager-app/components/cohorts/create-cohort.tsx(1 hunks)netmanager-app/components/grids/create-grid.tsx(1 hunks)netmanager-app/components/grids/date-range-picker.tsx(1 hunks)netmanager-app/components/grids/grid-sites.tsx(1 hunks)netmanager-app/components/ui/textarea.tsx(1 hunks)
🔇 Additional comments (1)
netmanager-app/components/grids/create-grid.tsx (1)
139-144: Map placeholder needs implementation.The map component is currently a placeholder. Consider implementing the actual map component for polygon selection.
Would you like me to help implement the map component using a mapping library like Leaflet or Mapbox?
| <Calendar | ||
| initialFocus | ||
| mode="range" | ||
| defaultMonth={date?.from} | ||
| selected={date} | ||
| onSelect={setDate} | ||
| numberOfMonths={2} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date range validation.
The calendar component should include validation to prevent invalid date ranges (e.g., "to" date before "from" date).
<Calendar
initialFocus
mode="range"
defaultMonth={date?.from}
selected={date}
- onSelect={setDate}
+ onSelect={(newDate) => {
+ if (newDate?.from && newDate.to && newDate.from > newDate.to) {
+ return; // Prevent invalid range selection
+ }
+ setDate(newDate);
+ onDateRangeChange?.(newDate);
+ }}
numberOfMonths={2}
+ disabled={{ before: new Date() }} // Optionally prevent past dates
/>Committable suggestion skipped: line range outside the PR's diff.
| const sites = [ | ||
| { | ||
| name: "McCarthy Square, Banjul", | ||
| siteId: "site_495", | ||
| parish: "", | ||
| subCounty: "", | ||
| city: "Serrekunda", | ||
| district: "Bakau", | ||
| region: "Kanifing", | ||
| country: "The Gambia", | ||
| }, | ||
| { | ||
| name: "Kanifing", | ||
| siteId: "site_496", | ||
| parish: "", | ||
| subCounty: "", | ||
| city: "Serrekunda", | ||
| district: "Bakau", | ||
| region: "Kanifing", | ||
| country: "The Gambia", | ||
| }, | ||
| { | ||
| name: "Westfield Junction", | ||
| siteId: "site_497", | ||
| parish: "", | ||
| subCounty: "", | ||
| city: "Serrekunda", | ||
| district: "Bakau", | ||
| region: "Kanifing", | ||
| country: "The Gambia", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move sample data to a separate file or replace with API call.
Hardcoded sample data should be moved to a separate file or replaced with an API call for better maintainability.
-// Sample data
-const sites = [ ... ];
+import { sites } from '@/data/sample-sites';
+// TODO: Replace with API call
+// const sites = await fetchSites();Committable suggestion skipped: line range outside the PR's diff.
| shapefile: '{"type":"","coordinates":[]}', | ||
| }, |
There was a problem hiding this comment.
Console.log in production code.
The onSubmit handler only logs the form data to the console. Consider implementing the actual grid creation logic.
function onSubmit(data: GridFormValues) {
- console.log(data);
+ // TODO: Implement grid creation logic
+ try {
+ // await createGrid(data);
+ toast.success('Grid created successfully');
setOpen(false);
+ } catch (error) {
+ toast.error('Failed to create grid');
+ }
}Also applies to: 55-58
| function onSubmit(values: z.infer<typeof formSchema>) { | ||
| console.log(values); | ||
| setOpen(false); | ||
| } |
There was a problem hiding this comment.
Console.log in production code.
The onSubmit handler only logs the form data to the console. Implement the actual device assignment logic.
function onSubmit(values: z.infer<typeof formSchema>) {
- console.log(values);
+ // TODO: Implement device assignment logic
+ try {
+ // await assignDevicesToCohort(cohortId, values.devices);
+ toast.success('Devices assigned successfully');
setOpen(false);
+ } catch (error) {
+ toast.error('Failed to assign devices');
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const devices = [ | ||
| { value: "aq_40", label: "Aq_40" }, | ||
| { value: "aq_41", label: "Aq_41" }, | ||
| { value: "aq_42", label: "Aq_42" }, | ||
| { value: "airqo_g5364", label: "Airqo_g5364" }, | ||
| ]; |
There was a problem hiding this comment.
Replace hardcoded devices with API data.
The devices array is currently hardcoded. Consider fetching this data from an API endpoint.
-const devices = [
- { value: "aq_40", label: "Aq_40" },
- { value: "aq_41", label: "Aq_41" },
- { value: "aq_42", label: "Aq_42" },
- { value: "airqo_g5364", label: "Airqo_g5364" },
-];
+const useDevices = () => {
+ const [devices, setDevices] = useState([]);
+
+ useEffect(() => {
+ const fetchDevices = async () => {
+ try {
+ // const response = await fetch('/api/devices');
+ // const data = await response.json();
+ // setDevices(data.map(d => ({ value: d.id, label: d.name })));
+ } catch (error) {
+ console.error('Failed to fetch devices:', error);
+ }
+ };
+
+ fetchDevices();
+ }, []);
+
+ return devices;
+};Committable suggestion skipped: line range outside the PR's diff.
| const handleReset = () => { | ||
| // Reset form to original values | ||
| }; | ||
|
|
||
| const handleSave = () => { | ||
| // Save changes | ||
| }; |
There was a problem hiding this comment.
Implement the placeholder functions.
The handleReset and handleSave functions are currently empty. This could lead to user confusion as the buttons appear functional but don't perform any actions.
- const handleReset = () => {
- // Reset form to original values
- };
+ const handleReset = () => {
+ setGridData(initialGridData);
+ };
- const handleSave = () => {
- // Save changes
- };
+ const handleSave = async () => {
+ try {
+ const response = await fetch(`/api/grids/${gridData.id}`, {
+ method: 'PUT',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ body: JSON.stringify(gridData),
+ });
+ if (!response.ok) throw new Error('Failed to save changes');
+ } catch (error) {
+ console.error('Error saving grid data:', error);
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
| const [gridData, setGridData] = useState({ | ||
| name: "gambia", | ||
| id: "673cf5b9328bd600130351c4", | ||
| visibility: "true", | ||
| administrativeLevel: "country", | ||
| description: "", | ||
| }); |
There was a problem hiding this comment.
Initialize state using environment variables or API calls.
The grid data is currently hardcoded. Consider fetching this data from an API endpoint or environment variables.
- const [gridData, setGridData] = useState({
- name: "gambia",
- id: "673cf5b9328bd600130351c4",
- visibility: "true",
- administrativeLevel: "country",
- description: "",
- });
+ const [gridData, setGridData] = useState(null);
+ const [isLoading, setIsLoading] = useState(true);
+
+ useEffect(() => {
+ const fetchGridData = async () => {
+ try {
+ const response = await fetch(`/api/grids/${params.id}`);
+ const data = await response.json();
+ setGridData(data);
+ } catch (error) {
+ console.error('Failed to fetch grid data:', error);
+ } finally {
+ setIsLoading(false);
+ }
+ };
+
+ fetchGridData();
+ }, [params.id]);Committable suggestion skipped: line range outside the PR's diff.
| const cohortData = { | ||
| name: "victoria_sugar", | ||
| id: "675bd462c06188001333d4d5", | ||
| visibility: "true", | ||
| }; | ||
|
|
||
| // Sample devices data | ||
| const devices = [ | ||
| { | ||
| name: "Aq_29", | ||
| description: "AIRQO UNIT with PMS5003 Victoria S", | ||
| site: "N/A", | ||
| deploymentStatus: "Deployed", | ||
| dateCreated: "2019-03-02T00:00:00.000Z", | ||
| }, | ||
| { | ||
| name: "Aq_34", | ||
| description: "AIRQO UNIT with PMS5003 Victoria S", | ||
| site: "N/A", | ||
| deploymentStatus: "Deployed", | ||
| dateCreated: "2019-03-14T00:00:00.000Z", | ||
| }, | ||
| { | ||
| name: "Aq_35", | ||
| description: "AIRQO UNIT with PMS5003 Victoria S", | ||
| site: "N/A", | ||
| deploymentStatus: "Deployed", | ||
| dateCreated: "2019-03-28T00:00:00.000Z", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Replace sample data with API integration.
The component uses hardcoded sample data. This should be replaced with actual data from an API endpoint.
-// Sample cohort data
-const cohortData = {
- name: "victoria_sugar",
- id: "675bd462c06188001333d4d5",
- visibility: "true",
-};
-
-// Sample devices data
-const devices = [
- {
- name: "Aq_29",
- description: "AIRQO UNIT with PMS5003 Victoria S",
- site: "N/A",
- deploymentStatus: "Deployed",
- dateCreated: "2019-03-02T00:00:00.000Z",
- },
- // ...
-];
+interface Device {
+ name: string;
+ description: string;
+ site: string;
+ deploymentStatus: string;
+ dateCreated: string;
+}
+
+interface CohortData {
+ name: string;
+ id: string;
+ visibility: string;
+}Committable suggestion skipped: line range outside the PR's diff.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="text-destructive hover:text-destructive" | ||
| > | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> | ||
| </TableCell> |
There was a problem hiding this comment.
Add confirmation dialog for device deletion.
The delete button lacks a confirmation dialog, which could lead to accidental deletions.
+import {
+ AlertDialog,
+ AlertDialogAction,
+ AlertDialogCancel,
+ AlertDialogContent,
+ AlertDialogDescription,
+ AlertDialogFooter,
+ AlertDialogHeader,
+ AlertDialogTitle,
+ AlertDialogTrigger,
+} from "@/components/ui/alert-dialog";
-<Button
- variant="ghost"
- size="icon"
- className="text-destructive hover:text-destructive"
->
- <Trash2 className="h-4 w-4" />
-</Button>
+<AlertDialog>
+ <AlertDialogTrigger asChild>
+ <Button
+ variant="ghost"
+ size="icon"
+ className="text-destructive hover:text-destructive"
+ >
+ <Trash2 className="h-4 w-4" />
+ </Button>
+ </AlertDialogTrigger>
+ <AlertDialogContent>
+ <AlertDialogHeader>
+ <AlertDialogTitle>Are you sure?</AlertDialogTitle>
+ <AlertDialogDescription>
+ This action cannot be undone. This will permanently remove the device
+ from this cohort.
+ </AlertDialogDescription>
+ </AlertDialogHeader>
+ <AlertDialogFooter>
+ <AlertDialogCancel>Cancel</AlertDialogCancel>
+ <AlertDialogAction onClick={() => handleDeviceDelete(device.name)}>
+ Delete
+ </AlertDialogAction>
+ </AlertDialogFooter>
+ </AlertDialogContent>
+</AlertDialog>Committable suggestion skipped: line range outside the PR's diff.
| const filteredDevices = devices.filter((device) => | ||
| Object.values(device).some((value) => | ||
| String(value).toLowerCase().includes(searchQuery.toLowerCase()) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize device filtering for better performance.
The current filtering implementation checks every value of every device object, which could be inefficient for large datasets. Consider filtering only on relevant fields.
- const filteredDevices = devices.filter((device) =>
- Object.values(device).some((value) =>
- String(value).toLowerCase().includes(searchQuery.toLowerCase())
- )
- );
+ const filteredDevices = devices.filter((device) => {
+ const searchLower = searchQuery.toLowerCase();
+ return (
+ device.name.toLowerCase().includes(searchLower) ||
+ device.description.toLowerCase().includes(searchLower) ||
+ device.site.toLowerCase().includes(searchLower)
+ );
+ });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx (4)
Line range hint
39-61: Consider enhancing coordinate validation precisionThe current validation ensures basic range checks for coordinates, but consider adding:
- Maximum length for site name to prevent database issues
- Decimal precision limits for coordinates (typically 6-7 decimal places)
const siteFormSchema = z.object({ - name: z.string().min(2, { + name: z.string().min(2, { message: "Site name must be at least 2 characters.", - }), + }).max(100, { + message: "Site name must not exceed 100 characters.", + }), latitude: z.string().refine( (val) => { const num = parseFloat(val); - return !isNaN(num) && num >= -90 && num <= 90; + return !isNaN(num) && num >= -90 && num <= 90 && + val.split('.')[1]?.length <= 6; }, { - message: "Latitude must be a valid number between -90 and 90", + message: "Latitude must be a valid number between -90 and 90 with max 6 decimal places", } ), // Similar changes for longitude
Line range hint
89-124: Consider enhancing map resilience and customizationThe map implementation is solid, but consider these improvements:
- Add an error boundary for map rendering failures
- Make zoom level configurable based on area context
+const DEFAULT_ZOOM = 13; + +class MapErrorBoundary extends React.Component { + state = { hasError: false }; + static getDerivedStateFromError() { + return { hasError: true }; + } + render() { + if (this.state.hasError) { + return ( + <div className="w-full h-64 bg-gray-200 flex items-center justify-center rounded-md"> + <p className="text-gray-500">Failed to load map. Please try again.</p> + </div> + ); + } + return this.props.children; + } +} return ( <div className="w-full h-64 rounded-md overflow-hidden"> + <MapErrorBoundary> <MapContainer center={position} - zoom={13} + zoom={DEFAULT_ZOOM} scrollWheelZoom={false} style={{ height: "100%", width: "100%" }} > {/* ... */} </MapContainer> + </MapErrorBoundary> </div> );
Line range hint
234-268: Consider optimizing coordinate updates and error handlingThe coordinate optimization feature is great, but consider these improvements:
- Debounce coordinate optimization to prevent rapid API calls
- Add more specific error handling for different failure scenarios
+import { useDebouncedCallback } from 'use-debounce'; export function CreateSiteForm() { // ... existing code ... + const debouncedOptimize = useDebouncedCallback( + (lat: string, lng: string) => { + getApproximateCoordinates( + { latitude: lat, longitude: lng }, + { + onSuccess: (data) => { + const { approximate_latitude, approximate_longitude } = + data.approximate_coordinates; + form.setValue("latitude", approximate_latitude.toString()); + form.setValue("longitude", approximate_longitude.toString()); + }, + onError: (error) => { + setError( + error.response?.status === 429 + ? "Too many optimization requests. Please try again later." + : "Failed to optimize coordinates. Please try again." + ); + }, + } + ); + }, + 1000 + );
Line range hint
269-385: Enhance accessibility and user experienceThe UI flow is intuitive, but consider these UX improvements:
- Add keyboard navigation support for map interactions
- Implement an unsaved changes warning when closing the dialog
Here's how you might implement the unsaved changes warning:
export function CreateSiteForm() { const [open, setOpen] = useState(false); + const formIsDirty = Object.keys(form.formState.dirtyFields).length > 0; + + const handleDialogClose = () => { + if (formIsDirty) { + if (window.confirm("You have unsaved changes. Are you sure you want to close?")) { + setOpen(false); + form.reset(); + } + } else { + setOpen(false); + } + }; return ( - <Dialog open={open} onOpenChange={setOpen}> + <Dialog open={open} onOpenChange={handleDialogClose}>netmanager-app/core/redux/slices/gridsSlice.ts (1)
12-15: Consider adding validation for polygon coordinates.While the type structure is good, consider adding runtime validation to ensure coordinate values are within valid ranges (longitude: -180 to 180, latitude: -90 to 90).
// Add a validation utility export const isValidPosition = (pos: Position): boolean => { const [lon, lat] = pos; return lon >= -180 && lon <= 180 && lat >= -90 && lat <= 90; };netmanager-app/components/grids/polymap.tsx (2)
96-101: Consider making map configuration more flexible.The center coordinates and zoom level are hardcoded. Consider making these configurable through props.
+interface PolygonMapProps { + center?: { lat: number; lng: number }; + zoom?: number; +} -const PolygonMap: React.FC = () => { +const PolygonMap: React.FC<PolygonMapProps> = ({ + center = { lat: 0.347596, lng: 32.58252 }, + zoom = 10 +}) => {
64-91: Consider adding error boundaries for map operations.The event handlers don't have try-catch blocks for error handling. Consider adding error boundaries to handle potential Leaflet errors gracefully.
// Add an error boundary component class MapErrorBoundary extends React.Component { // Implementation details... } // Wrap the MapContainer with the error boundary <MapErrorBoundary> <MapContainer {...props} /> </MapErrorBoundary>netmanager-app/components/grids/create-grid.tsx (2)
147-153: Consider improving the shapefile input UX.The disabled textarea showing raw JSON might not be the most user-friendly approach. Consider adding a preview component or a more intuitive way to display the selected polygon data.
171-171: Add loading state to the submit button.The submit button should reflect the loading state to prevent double submissions.
- <Button type="submit">Submit</Button> + <Button type="submit" disabled={isLoading}> + {isLoading ? ( + <> + <Loader2 className="mr-2 h-4 w-4 animate-spin" /> + Creating... + </> + ) : ( + 'Submit' + )} + </Button>netmanager-app/app/(authenticated)/grids/page.tsx (3)
31-35: Consider debouncing the search filter.The current implementation filters on every keystroke, which might cause performance issues with large datasets.
+import { useMemo } from "react"; +import { useDebounce } from "@/hooks/useDebounce"; export default function GridsPage() { // ... existing code ... + const debouncedSearch = useDebounce(searchQuery, 300); - const filteredGrids = grids.filter( + const filteredGrids = useMemo(() => grids.filter( (grid) => - grid.name.toLowerCase().includes(searchQuery.toLowerCase()) || - grid.admin_level.toLowerCase().includes(searchQuery.toLowerCase()) - ); + grid.name.toLowerCase().includes(debouncedSearch.toLowerCase()) || + grid.admin_level.toLowerCase().includes(debouncedSearch.toLowerCase()) + ), [grids, debouncedSearch]);
175-178: Add hover state and cursor pointer to table rows.Since the rows are clickable, they should have visual feedback on hover.
<TableRow key={grid._id} onClick={() => router.push(`/grids/${grid._id}`)} + className="cursor-pointer hover:bg-muted/50" >
122-128: Consider adding a skeleton loader.Replace the centered spinner with a skeleton loader to prevent layout shift.
if (isGridsLoading) { return ( - <div className="flex items-center justify-center min-h-screen"> - <Loader2 className="w-8 h-8 animate-spin" /> + <div className="p-6 space-y-6"> + <div className="space-y-4"> + <div className="h-8 w-48 bg-muted animate-pulse rounded" /> + <div className="h-4 w-64 bg-muted animate-pulse rounded" /> + </div> + <div className="space-y-4"> + {Array.from({ length: 5 }).map((_, i) => ( + <div key={i} className="h-16 bg-muted animate-pulse rounded" /> + ))} + </div> </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
netmanager-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
netmanager-app/app/(authenticated)/grids/page.tsx(1 hunks)netmanager-app/app/(authenticated)/sites/create-site-form.tsx(1 hunks)netmanager-app/app/types/grids.ts(1 hunks)netmanager-app/components/grids/create-grid.tsx(1 hunks)netmanager-app/components/grids/polymap.tsx(1 hunks)netmanager-app/core/hooks/useGrids.ts(1 hunks)netmanager-app/core/redux/slices/gridsSlice.ts(2 hunks)netmanager-app/package.json(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/grids/polymap.tsx
[error] 41-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
netmanager-app/components/grids/create-grid.tsx
[error] 88-88: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
🔇 Additional comments (8)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx (1)
28-28: Nice choice of icons for loading and success states! 👍The Loader2 and Check icons provide clear visual feedback for form submission states.
netmanager-app/app/types/grids.ts (1)
9-9: Great improvement in type safety!The change from
number[][][][]toPosition[][] | Position[][][]provides better type safety and clarity by using thePositiontype for coordinates. The union type correctly handles both Polygon and MultiPolygon geometries.netmanager-app/core/redux/slices/gridsSlice.ts (2)
5-5: Well-documented type definition!The
Positiontype with its documentation clearly indicates the coordinate order (longitude, latitude), which is crucial for geospatial data handling.
47-55: Consider adding error handling for invalid coordinates.The
setPolygonreducer directly sets the state without validation. Consider adding error handling for invalid polygon data.netmanager-app/core/hooks/useGrids.ts (2)
107-107: Good switch to async mutation!The change to
mutateAsyncenables better error handling and allows components to handle the mutation result using async/await.
99-99: Verify cache invalidation strategy.The query key has been changed from "gridSummary" to "grids". Ensure all components using this cache key have been updated accordingly.
✅ Verification successful
Cache invalidation strategy is correctly implemented ✅
The query key change is consistent throughout the codebase. The invalidation strategy using the parent key
["grids"]will properly invalidate all grid-related queries, including network-specific ones.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old query key rg "gridSummary" --type tsLength of output: 26
Script:
#!/bin/bash # Search for React Query hook usage and invalidation patterns ast-grep --pattern 'useQuery($$$"grids"$$$)' ast-grep --pattern 'invalidateQueries($$$"grids"$$$)' # Search for any string references to "grids" in TypeScript files rg '"grids"' --type ts -C 2Length of output: 1229
netmanager-app/package.json (1)
45-45: LGTM! Good job moving type definitions to devDependencies.The changes look good. Moving
@types/leafletto devDependencies is the correct approach since type definitions are only needed during development.Let's verify the version compatibility between the packages:
Also applies to: 56-56
✅ Verification successful
Version compatibility verified and type definitions are correctly placed ✅
The package versions are compatible:
- leaflet ^1.9.4 matches its type definitions ^1.9.16
- react-leaflet ^4.2.1 and react-leaflet-draw ^0.20.4 are compatible with the leaflet version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check version compatibility between leaflet-related packages # Check installed versions npm list leaflet react-leaflet react-leaflet-draw @types/leaflet # Check latest versions and their dependencies npm view leaflet@latest react-leaflet@latest react-leaflet-draw@latest @types/leaflet@latest version peerDependenciesLength of output: 220
Script:
#!/bin/bash # Get all leaflet-related dependencies from package.json rg "leaflet" -A 1 -B 1 package.json # Also check for any potential peer dependency issues in package.json cat package.jsonLength of output: 152
Script:
#!/bin/bash # Get all leaflet-related dependencies from the correct package.json rg "leaflet" -A 1 -B 1 netmanager-app/package.json # Also check the full package.json content cat netmanager-app/package.jsonLength of output: 2349
netmanager-app/app/(authenticated)/grids/page.tsx (1)
140-143: Implement the export functionality.The Export button is currently non-functional.
| // Fix Leaflet's default icon issue | ||
| delete (L.Icon.Default.prototype as IconDefault)._getIconUrl; | ||
| L.Icon.Default.mergeOptions({ | ||
| iconRetinaUrl: | ||
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon-2x.png", | ||
| iconUrl: | ||
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon.png", | ||
| shadowUrl: | ||
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-shadow.png", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor icon configuration for better performance.
The delete operator usage is flagged for potential performance impact. Consider refactoring the icon configuration.
-delete (L.Icon.Default.prototype as IconDefault)._getIconUrl;
+((L.Icon.Default.prototype as IconDefault)._getIconUrl as unknown) = undefined;
+// Move URLs to constants
+const ICON_URLS = {
+ iconRetinaUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon-2x.png",
+ iconUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon.png",
+ shadowUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-shadow.png"
+};
-L.Icon.Default.mergeOptions({
- iconRetinaUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon-2x.png",
- iconUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon.png",
- shadowUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-shadow.png",
-});
+L.Icon.Default.mergeOptions(ICON_URLS);📝 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.
| // Fix Leaflet's default icon issue | |
| delete (L.Icon.Default.prototype as IconDefault)._getIconUrl; | |
| L.Icon.Default.mergeOptions({ | |
| iconRetinaUrl: | |
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon-2x.png", | |
| iconUrl: | |
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon.png", | |
| shadowUrl: | |
| "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-shadow.png", | |
| }); | |
| // Fix Leaflet's default icon issue | |
| ((L.Icon.Default.prototype as IconDefault)._getIconUrl as unknown) = undefined; | |
| // Move URLs to constants | |
| const ICON_URLS = { | |
| iconRetinaUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon-2x.png", | |
| iconUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-icon.png", | |
| shadowUrl: "https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.4/images/marker-shadow.png" | |
| }; | |
| L.Icon.Default.mergeOptions(ICON_URLS); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| await createGrid(gridData); | ||
|
|
||
| setOpen(false); | ||
| } catch (error: AxiosError<ErrorResponse>) { |
There was a problem hiding this comment.
Fix the type annotation in the catch clause.
The catch clause variable type annotation should be unknown for better type safety.
- } catch (error: AxiosError<ErrorResponse>) {
+ } catch (error: unknown) {
+ if (error instanceof AxiosError) {
setError(error.message || "An error occurred while creating the site.");
+ } else {
+ setError("An unexpected error occurred.");
+ }📝 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.
| } catch (error: AxiosError<ErrorResponse>) { | |
| } catch (error: unknown) { | |
| if (error instanceof AxiosError) { | |
| setError(error.message || "An error occurred while creating the site."); | |
| } else { | |
| setError("An unexpected error occurred."); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
netmanager-app/app/(authenticated)/grids/[id]/page.tsx (2)
22-32: Add TypeScript interface for grid data.Define a proper TypeScript interface for the grid data structure to improve type safety and code maintainability.
interface GridData { name: string; id: string; _id?: string; visibility: string; administrativeLevel: string; description: string; }
44-46: Add error handling for clipboard operations.The clipboard operation should handle potential errors and provide user feedback.
- const handleCopyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); + const handleCopyToClipboard = async (text: string) => { + try { + await navigator.clipboard.writeText(text); + // TODO: Show success toast + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // TODO: Show error toast + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netmanager-app/app/(authenticated)/grids/[id]/page.tsx(1 hunks)netmanager-app/core/hooks/useGrids.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager-app/core/hooks/useGrids.ts
🔇 Additional comments (1)
netmanager-app/app/(authenticated)/grids/[id]/page.tsx (1)
34-42:⚠️ Potential issueFix incomplete dependency array in useEffect.
The useEffect hook's dependency array is missing individual properties from gridDetails, which could cause stale state updates.
useEffect(() => { setGridData({ ...gridDetails, name: gridDetails.name, visibility: gridDetails.visibility, administrativeLevel: gridDetails.admin_level, description: gridDetails.description, }); - }, [gridDetails]); + }, [ + gridDetails, + gridDetails.name, + gridDetails.visibility, + gridDetails.admin_level, + gridDetails.description + ]);Likely invalid or redundant comment.
| {/* Grid Details Form */} | ||
| <div className="grid gap-6"> | ||
| <div className="grid gap-4 md:grid-cols-2"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="gridName">Grid name *</Label> | ||
| <Input | ||
| id="gridName" | ||
| value={gridData.name} | ||
| onChange={(e) => | ||
| setGridData({ ...gridData, name: e.target.value }) | ||
| } | ||
| /> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="gridId">Grid ID *</Label> | ||
| <div className="flex gap-2"> | ||
| <Input id="gridId" value={gridData._id} readOnly /> | ||
| <Button | ||
| variant="outline" | ||
| size="icon" | ||
| onClick={() => handleCopyToClipboard(gridData.id)} | ||
| > | ||
| <Copy className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="grid gap-4 md:grid-cols-2"> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="visibility">Visibility *</Label> | ||
| <Select | ||
| value={gridData.visibility} | ||
| onValueChange={(value) => | ||
| setGridData({ ...gridData, visibility: value }) | ||
| } | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select visibility" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="true">True</SelectItem> | ||
| <SelectItem value="false">False</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="adminLevel">Administrative level *</Label> | ||
| <Input | ||
| id="adminLevel" | ||
| value={gridData.administrativeLevel} | ||
| onChange={(e) => | ||
| setGridData({ | ||
| ...gridData, | ||
| administrativeLevel: e.target.value, | ||
| }) | ||
| } | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <Label htmlFor="description">Description</Label> | ||
| <Textarea | ||
| id="description" | ||
| value={gridData.description} | ||
| onChange={(e) => | ||
| setGridData({ ...gridData, description: e.target.value }) | ||
| } | ||
| rows={4} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add form validation for required fields.
The form marks fields as required (*) but lacks proper validation before saving.
interface ValidationErrors {
name?: string;
visibility?: string;
administrativeLevel?: string;
}
const validateForm = (): ValidationErrors | null => {
const errors: ValidationErrors = {};
if (!gridData.name.trim()) {
errors.name = 'Grid name is required';
}
if (!gridData.visibility) {
errors.visibility = 'Visibility is required';
}
if (!gridData.administrativeLevel.trim()) {
errors.administrativeLevel = 'Administrative level is required';
}
return Object.keys(errors).length ? errors : null;
};Add error messages below each required field and validate before saving:
const handleSave = () => {
- // Save changes
+ const errors = validateForm();
+ if (errors) {
+ // TODO: Show validation errors
+ return;
+ }
+ // Proceed with saving
};| if (isLoading) { | ||
| return ( | ||
| <div className="flex items-center justify-center min-h-screen"> | ||
| <Loader2 className="w-8 h-8 animate-spin" /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error state handling.
The component retrieves an error from useGridDetails but doesn't handle it in the UI.
if (isLoading) {
return (
<div className="flex items-center justify-center min-h-screen">
<Loader2 className="w-8 h-8 animate-spin" />
</div>
);
}
+
+ if (error) {
+ return (
+ <div className="flex items-center justify-center min-h-screen text-destructive">
+ <p>Failed to load grid details: {error.message}</p>
+ </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.
| if (isLoading) { | |
| return ( | |
| <div className="flex items-center justify-center min-h-screen"> | |
| <Loader2 className="w-8 h-8 animate-spin" /> | |
| </div> | |
| ); | |
| } | |
| if (isLoading) { | |
| return ( | |
| <div className="flex items-center justify-center min-h-screen"> | |
| <Loader2 className="w-8 h-8 animate-spin" /> | |
| </div> | |
| ); | |
| } | |
| if (error) { | |
| return ( | |
| <div className="flex items-center justify-center min-h-screen text-destructive"> | |
| <p>Failed to load grid details: {error.message}</p> | |
| </div> | |
| ); | |
| } |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
CohortDetailsPageandCohortsPagefor cohort management.GridDetailsPageandGridsPagefor grid management.Improvements