Added devices overview page, sites coordinates generator(site creation), network dropdown, updated organisation dropdown #2375
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the netmanager-app, focusing on configuration, device management, and user interface improvements. The changes include adding new environment variables, implementing device overview functionality, introducing a toaster notification system, and creating new API and Redux slice for device management. The modifications span across configuration files, components, hooks, and state management to provide a more robust and interactive application experience. Changes
Possibly related PRs
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: 9
🧹 Nitpick comments (12)
netmanager-app/core/apis/devices.ts (1)
19-24: Improve error handling robustnessThe current error handling could be more informative and robust.
Consider this enhancement:
- const axiosError = error as AxiosError<ErrorResponse>; - throw new Error( - axiosError.response?.data?.message || "Failed to fetch devices summary" - ); + if (error instanceof AxiosError) { + const message = error.response?.data?.message || error.message; + throw new Error(`Failed to fetch devices summary: ${message}`); + } + throw error;netmanager-app/components/ui/sonner.tsx (1)
8-29: Enhance type safety and code organizationThe component implementation looks good, but could benefit from some TypeScript enhancements and code organization improvements.
Consider these improvements:
+ const TOAST_CLASSES = { + toast: "group toast group-[.toaster]:bg-background group-[.toaster]:text-foreground group-[.toaster]:border-border group-[.toaster]:shadow-lg", + description: "group-[.toast]:text-muted-foreground", + actionButton: "group-[.toast]:bg-primary group-[.toast]:text-primary-foreground", + cancelButton: "group-[.toast]:bg-muted group-[.toast]:text-muted-foreground", + } as const; - const Toaster = ({ ...props }: ToasterProps) => { + const Toaster = ({ ...props }: ToasterProps): JSX.Element => { const { theme = "system" } = useTheme(); return ( <Sonner theme={theme as ToasterProps["theme"]} className="toaster group" toastOptions={{ - classNames: { - toast: "group toast group-[.toaster]:bg-background group-[.toaster]:text-foreground group-[.toaster]:border-border group-[.toaster]:shadow-lg", - description: "group-[.toast]:text-muted-foreground", - actionButton: "group-[.toast]:bg-primary group-[.toast]:text-primary-foreground", - cancelButton: "group-[.toast]:bg-muted group-[.toast]:text-muted-foreground", - }, + classNames: TOAST_CLASSES, }} {...props} /> ); };netmanager-app/components/ui/tooltip.tsx (1)
1-30: Clean implementation of Radix UI tooltip components! 👍The implementation is well-structured and follows React best practices. The use of Radix UI primitives ensures good accessibility out of the box.
Consider adding
aria-labelsupport through props to enhance accessibility further:const TooltipContent = React.forwardRef< React.ElementRef<typeof TooltipPrimitive.Content>, React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Content> ->(({ className, sideOffset = 4, ...props }, ref) => ( +>(({ className, sideOffset = 4, "aria-label": ariaLabel, ...props }, ref) => ( <TooltipPrimitive.Content ref={ref} sideOffset={sideOffset} + aria-label={ariaLabel} className={cn(netmanager-app/core/redux/slices/devicesSlice.ts (1)
5-9: Consider expanding the DevicesState interfaceThe current state interface could benefit from additional fields to handle common state management scenarios.
Consider adding these fields to improve state management:
interface DevicesState { devices: Device[]; error: string | null; selectedDevice: Device | null; + isLoading: boolean; + pagination: { + currentPage: number; + totalPages: number; + pageSize: number; + }; + filters: { + status?: string; + category?: string; + isOnline?: boolean; + }; }netmanager-app/app/types/devices.ts (2)
11-22: Enhance site_category type safetyThe site_category object could benefit from more specific types and validation.
Consider adding these improvements:
site_category: { - tags: string[]; + tags: readonly string[]; area_name: string; - category: string; + category: "residential" | "commercial" | "industrial" | "rural"; highway: string; landuse: string; - latitude: number; - longitude: number; + latitude: Readonly<number & { _brand: "latitude" }>; + longitude: Readonly<number & { _brand: "longitude" }>; natural: string; - search_radius: number; + search_radius: number & { _brand: "meters" }; waterway: string; };
34-56: Add validation constraints to Device interfaceSome fields in the Device interface could benefit from more specific types and validation.
Consider adding these constraints:
export interface Device { _id: string; isOnline: boolean; - device_codes: string[]; + device_codes: readonly string[]; status: string; - category: string; + category: "monitor" | "calibrator" | "other"; isActive: boolean; description: string; name: string; network: string; long_name: string; - createdAt: string; + createdAt: `${number}-${number}-${number}T${number}:${number}:${number}Z`; authRequired: boolean; serial_number: string; api_code: string; - latitude: number; - longitude: number; + latitude: number & { _brand: "latitude" }; + longitude: number & { _brand: "longitude" };netmanager-app/core/hooks/useSites.ts (1)
35-56: Consider using a more specific error type.While the implementation is solid, consider defining a specific error type for coordinate-related errors to improve error handling predictability.
} = useMutation< ApproximateCoordinatesResponse, - Error, + { code: string; message: string }, { latitude: string; longitude: string } >({netmanager-app/app/(authenticated)/sites/create-site-form.tsx (1)
270-314: Add error handling and coordinate validation.The optimize location feature works well but could be improved:
- Add error toast notifications for failed optimizations
- Validate coordinate format before optimization
onClick={() => { const lat = form.getValues("latitude"); const lng = form.getValues("longitude"); - if (lat && lng) { + const isValidCoord = (coord: string) => /^-?\d+\.?\d*$/.test(coord); + if (lat && lng && isValidCoord(lat) && isValidCoord(lng)) { 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() ); + toast.success("Location coordinates optimized successfully"); }, + onError: (error) => { + toast.error(error.message || "Failed to optimize coordinates"); + }, } ); + } else { + toast.error("Please enter valid latitude and longitude values"); } }}netmanager-app/app/(authenticated)/devices/overview/page.tsx (3)
66-86: Improve date handling in sort function.The current date handling could be more robust by using a proper date parsing utility.
if (sortField === "createdAt") { - const dateA = new Date(a.createdAt || 0).getTime(); - const dateB = new Date(b.createdAt || 0).getTime(); + // Use a library like date-fns for more robust date parsing + const parseDate = (date: string | null) => { + if (!date) return 0; + const parsed = Date.parse(date); + return isNaN(parsed) ? 0 : parsed; + }; + const dateA = parseDate(a.createdAt); + const dateB = parseDate(b.createdAt); return sortOrder === "asc" ? dateA - dateB : dateB - dateA; }
88-99: Extract search fields configuration for better maintainability.Consider moving the searchable fields into a configuration object for easier maintenance.
+const SEARCHABLE_FIELDS = { + direct: ['name', 'long_name', '_id', 'description'], + array: [{ + field: 'device_codes', + searchFn: (codes: string[], query: string) => + codes.some(code => code.toLowerCase().includes(query)) + }] +}; const filteredDevices = devices.filter((device: Device) => { const searchLower = searchQuery.toLowerCase(); - return ( - device.name?.toLowerCase().includes(searchLower) || - device.long_name?.toLowerCase().includes(searchLower) || - device._id?.toLowerCase().includes(searchLower) || - device.description?.toLowerCase().includes(searchLower) || - device.device_codes.some((code) => - code.toLowerCase().includes(searchLower) - ) - ); + return SEARCHABLE_FIELDS.direct.some(field => + device[field]?.toLowerCase().includes(searchLower) + ) || SEARCHABLE_FIELDS.array.some(({ field, searchFn }) => + searchFn(device[field], searchLower) + ); });
321-337: Type device statuses and extract status styles configuration.Consider typing the status values and extracting the status styles for better maintainability.
+type DeviceStatus = 'deployed' | 'recalled' | 'not_deployed'; +const STATUS_STYLES: Record<DeviceStatus, { bg: string; text: string }> = { + deployed: { bg: 'bg-green-100', text: 'text-green-800' }, + recalled: { bg: 'bg-yellow-100', text: 'text-yellow-800' }, + not_deployed: { bg: 'bg-red-100', text: 'text-red-800' } +}; +const STATUS_LABELS: Record<DeviceStatus, string> = { + deployed: 'Deployed', + recalled: 'Recalled', + not_deployed: 'Not Deployed' +}; -className={`inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-medium ${ - device.status === "deployed" - ? "bg-green-100 text-green-800" - : device.status === "recalled" - ? "bg-yellow-100 text-yellow-800" - : "bg-red-100 text-red-800" -}`} +className={`inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-medium ${ + STATUS_STYLES[device.status as DeviceStatus].bg + STATUS_STYLES[device.status as DeviceStatus].text +}`} > -{device.status === "deployed" - ? "Deployed" - : device.status === "recalled" - ? "Recalled" - : "Not Deployed"} +{STATUS_LABELS[device.status as DeviceStatus]}netmanager-app/components/sidebar.tsx (1)
95-125: Consider accessibility improvements for the organization switcher.While the implementation is solid, consider adding ARIA labels and keyboard navigation support for better accessibility.
<Button variant="secondary" className="w-full justify-between uppercase" + aria-label={`Current organization: ${activeGroup?.grp_title || "None selected"}`} >
📜 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 (17)
netmanager-app/.env.example(1 hunks)netmanager-app/.gitignore(1 hunks)netmanager-app/app/(authenticated)/devices/overview/page.tsx(1 hunks)netmanager-app/app/(authenticated)/layout.tsx(1 hunks)netmanager-app/app/(authenticated)/sites/create-site-form.tsx(3 hunks)netmanager-app/app/types/devices.ts(1 hunks)netmanager-app/components/sidebar.tsx(7 hunks)netmanager-app/components/ui/sonner.tsx(1 hunks)netmanager-app/components/ui/tooltip.tsx(1 hunks)netmanager-app/core/apis/devices.ts(1 hunks)netmanager-app/core/apis/sites.ts(2 hunks)netmanager-app/core/hooks/useDevices.ts(1 hunks)netmanager-app/core/hooks/useSites.ts(2 hunks)netmanager-app/core/redux/slices/devicesSlice.ts(1 hunks)netmanager-app/core/redux/store.ts(1 hunks)netmanager-app/next.config.js(1 hunks)netmanager-app/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- netmanager-app/.gitignore
- netmanager-app/.env.example
🔇 Additional comments (12)
netmanager-app/next.config.js (2)
1-2: Clean type definition! 👍The JSDoc type annotation ensures proper TypeScript support for the Next.js configuration.
1-35: Verify configuration compatibilityLet's ensure this configuration works with the rest of your application setup.
✅ Verification successful
Configuration is compatible and well-isolated ✅
The Next.js configuration is properly scoped to the netmanager-app and won't conflict with other parts of the codebase. The development-only rewrite rule and CORS headers are safely implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for environment variable usage and related configuration files # Look for environment variable references echo "Checking environment variable usage..." rg "NEXT_PUBLIC_ENV|API_BASE_URL" --type js --type ts # Check for existing next.config.js in other directories echo "Checking for other Next.js configs..." fd "next.config.js" # Look for related environment files echo "Checking environment files..." fd ".env"Length of output: 3025
Script:
#!/bin/bash # Check platform next.config.js content echo "Checking platform next.config.js content..." cat platform/next.config.js # Look for staging-analytics.airqo.net references echo -e "\nChecking staging analytics URL references..." rg "staging-analytics.airqo.net" --type js --type ts --type jsonLength of output: 1350
netmanager-app/app/(authenticated)/layout.tsx (1)
5-5: Nice work on the Toaster integration!The placement of the Toaster component at the authenticated layout level is a solid choice, ensuring toast notifications are consistently available across all authenticated routes.
Also applies to: 12-17
netmanager-app/core/redux/store.ts (1)
4-4: Clean Redux store integration 👍The devices reducer integration follows the established pattern and maintains a consistent store structure.
Also applies to: 10-10
netmanager-app/core/hooks/useSites.ts (1)
7-19: LGTM! Well-structured query implementation.The hook correctly handles the dependency on both network and group context, with proper null checks in the enabled condition.
netmanager-app/core/apis/sites.ts (1)
7-22: Well-structured type definitions!The interfaces are well-defined with proper optional fields and clear structure.
netmanager-app/app/(authenticated)/devices/overview/page.tsx (1)
358-407: Well-implemented pagination logic!The pagination implementation handles edge cases properly and provides good user feedback.
netmanager-app/components/sidebar.tsx (4)
42-46: LGTM! Clean import organization.The imports are well-organized, with related Redux actions grouped together. The type imports are properly separated.
62-63: LGTM! Proper state management setup.The Redux selectors are correctly implemented using the
useAppSelectorhook.
88-91: LGTM! Consistent state management pattern.The
handleOrganizationChangefunction follows the same pattern ashandleNetworkChange, maintaining consistency in state management.
292-292: LGTM! Consistent path updates.The navigation paths have been consistently updated to use
/devices/overviewthroughout the component.Also applies to: 312-314
netmanager-app/package.json (1)
19-19: Verify dependency versions and check for security advisories.The new dependencies have been added with caret ranges, which is good for minor version updates. However, let's verify these versions for security and compatibility.
Also applies to: 32-32, 38-38
✅ Verification successful
✓ Dependencies are secure and version ranges are appropriate
The security check revealed no known vulnerabilities for @radix-ui/react-tooltip, next-themes, and sonner. The use of caret ranges (^) is appropriate for these UI components, allowing for safe minor version updates while maintaining compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories for the new dependencies echo "Checking @radix-ui/react-tooltip..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "@radix-ui/react-tooltip") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }' echo "Checking next-themes..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "next-themes") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }' echo "Checking sonner..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "sonner") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }'Length of output: 1059
| async headers() { | ||
| return [ | ||
| { | ||
| source: "/api/v2/:path*", | ||
| headers: [ | ||
| { key: "Access-Control-Allow-Origin", value: "*" }, | ||
| { | ||
| key: "Access-Control-Allow-Methods", | ||
| value: "GET,POST,PUT,DELETE,OPTIONS", | ||
| }, | ||
| { | ||
| key: "Access-Control-Allow-Headers", | ||
| value: "Content-Type, Authorization", | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
| }, |
There was a problem hiding this comment.
Security: Tighten CORS configuration
The current CORS configuration might be too permissive for production use:
- Using
*for Access-Control-Allow-Origin in production is not recommended. Consider restricting it to specific domains. - The allowed methods should be comma-separated with spaces for better compatibility.
Here's a suggested improvement:
headers: [
{
- key: "Access-Control-Allow-Origin",
- value: "*"
+ key: "Access-Control-Allow-Origin",
+ value: process.env.NODE_ENV === "development"
+ ? "*"
+ : process.env.ALLOWED_ORIGINS
},
{
key: "Access-Control-Allow-Methods",
- value: "GET,POST,PUT,DELETE,OPTIONS",
+ value: "GET, POST, PUT, DELETE, OPTIONS",
},Also consider adding these security headers:
{
key: 'Content-Security-Policy',
value: "default-src 'self';"
},
{
key: 'X-Frame-Options',
value: 'DENY'
}| async rewrites() { | ||
| if (process.env.NEXT_PUBLIC_ENV === "development") { | ||
| return [ | ||
| { | ||
| source: "/api/v2/:path*", | ||
| destination: "https://staging-analytics.airqo.net/api/v2/:path*", | ||
| }, | ||
| ]; | ||
| } | ||
| return []; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider these configuration improvements
- Using
NEXT_PUBLIC_prefix for the environment variable isn't recommended here since this is server-side configuration. Consider using a regular environment variable instead. - The staging URL should be configurable through environment variables rather than hardcoded.
Here's a suggested improvement:
- if (process.env.NEXT_PUBLIC_ENV === "development") {
+ if (process.env.NODE_ENV === "development") {
return [
{
source: "/api/v2/:path*",
- destination: "https://staging-analytics.airqo.net/api/v2/:path*",
+ destination: `${process.env.API_BASE_URL}/api/v2/:path*`,
},
];
}Don't forget to update your environment files to include API_BASE_URL.
📝 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.
| async rewrites() { | |
| if (process.env.NEXT_PUBLIC_ENV === "development") { | |
| return [ | |
| { | |
| source: "/api/v2/:path*", | |
| destination: "https://staging-analytics.airqo.net/api/v2/:path*", | |
| }, | |
| ]; | |
| } | |
| return []; | |
| }, | |
| async rewrites() { | |
| if (process.env.NODE_ENV === "development") { | |
| return [ | |
| { | |
| source: "/api/v2/:path*", | |
| destination: `${process.env.API_BASE_URL}/api/v2/:path*`, | |
| }, | |
| ]; | |
| } | |
| return []; | |
| }, |
| const response = await axiosInstance.get<DevicesSummaryResponse>( | ||
| `${DEVICES_MGT_URL}/summary?network=${networkId}&group=${groupName}` | ||
| ); |
There was a problem hiding this comment.
Enhance URL parameter safety
The URL parameters should be properly encoded to prevent potential injection vulnerabilities.
Here's how to fix it:
- `${DEVICES_MGT_URL}/summary?network=${networkId}&group=${groupName}`
+ `${DEVICES_MGT_URL}/summary?network=${encodeURIComponent(networkId)}&group=${encodeURIComponent(groupName)}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await axiosInstance.get<DevicesSummaryResponse>( | |
| `${DEVICES_MGT_URL}/summary?network=${networkId}&group=${groupName}` | |
| ); | |
| const response = await axiosInstance.get<DevicesSummaryResponse>( | |
| `${DEVICES_MGT_URL}/summary?network=${encodeURIComponent(networkId)}&group=${encodeURIComponent(groupName)}` | |
| ); |
| setDevices(state: DevicesState, action: PayloadAction<Device[]>) { | ||
| state.devices = action.payload; | ||
| state.error = null; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for edge cases
The setDevices reducer should handle potential edge cases.
Consider adding validation:
setDevices(state: DevicesState, action: PayloadAction<Device[]>) {
+ if (!Array.isArray(action.payload)) {
+ state.error = "Invalid devices data received";
+ return;
+ }
state.devices = action.payload;
state.error = null;
},📝 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.
| setDevices(state: DevicesState, action: PayloadAction<Device[]>) { | |
| state.devices = action.payload; | |
| state.error = null; | |
| }, | |
| setDevices(state: DevicesState, action: PayloadAction<Device[]>) { | |
| if (!Array.isArray(action.payload)) { | |
| state.error = "Invalid devices data received"; | |
| return; | |
| } | |
| state.devices = action.payload; | |
| state.error = null; | |
| }, |
| onSuccess: (data, error) => { | ||
| if (error) { | ||
| dispatch(setError(error.message)); | ||
| } else if (data) { | ||
| dispatch(setDevices(data.devices)); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fix incorrect error handling logic
The error handling in the onSuccess callback is logically incorrect. The onSuccess callback won't be called with an error parameter.
Implement proper error handling:
- onSuccess: (data, error) => {
- if (error) {
- dispatch(setError(error.message));
- } else if (data) {
- dispatch(setDevices(data.devices));
- }
+ onSuccess: (data) => {
+ dispatch(setDevices(data.devices));
},
+ onError: (error: Error) => {
+ dispatch(setError(error.message));
+ },📝 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.
| onSuccess: (data, error) => { | |
| if (error) { | |
| dispatch(setError(error.message)); | |
| } else if (data) { | |
| dispatch(setDevices(data.devices)); | |
| } | |
| }, | |
| onSuccess: (data) => { | |
| dispatch(setDevices(data.devices)); | |
| }, | |
| onError: (error: Error) => { | |
| dispatch(setError(error.message)); | |
| }, |
| const { data, isLoading, error } = useQuery<DevicesSummaryResponse, Error>({ | ||
| queryKey: ["devices", activeNetwork?.net_name, activeGroup?.grp_title], | ||
| queryFn: () => | ||
| devices.getDevicesSummary( | ||
| activeNetwork?.net_name || "", | ||
| activeGroup?.grp_title || "" | ||
| ), | ||
| enabled: !!activeNetwork?.net_name && !!activeGroup?.grp_title, | ||
| onSuccess: (data, error) => { | ||
| if (error) { | ||
| dispatch(setError(error.message)); | ||
| } else if (data) { | ||
| dispatch(setDevices(data.devices)); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add retry configuration and error boundaries
The query configuration could benefit from retry logic and better error handling.
Add retry configuration and validate network/group:
const { data, isLoading, error } = useQuery<DevicesSummaryResponse, Error>({
queryKey: ["devices", activeNetwork?.net_name, activeGroup?.grp_title],
queryFn: () => {
+ if (!activeNetwork?.net_name || !activeGroup?.grp_title) {
+ throw new Error("Network and group must be selected");
+ }
return devices.getDevicesSummary(
activeNetwork?.net_name || "",
activeGroup?.grp_title || ""
);
},
enabled: !!activeNetwork?.net_name && !!activeGroup?.grp_title,
+ retry: (failureCount, error) => {
+ return failureCount < 3 && !error.message.includes("Network and group must be selected");
+ },
+ retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data, isLoading, error } = useQuery<DevicesSummaryResponse, Error>({ | |
| queryKey: ["devices", activeNetwork?.net_name, activeGroup?.grp_title], | |
| queryFn: () => | |
| devices.getDevicesSummary( | |
| activeNetwork?.net_name || "", | |
| activeGroup?.grp_title || "" | |
| ), | |
| enabled: !!activeNetwork?.net_name && !!activeGroup?.grp_title, | |
| onSuccess: (data, error) => { | |
| if (error) { | |
| dispatch(setError(error.message)); | |
| } else if (data) { | |
| dispatch(setDevices(data.devices)); | |
| } | |
| }, | |
| }); | |
| const { data, isLoading, error } = useQuery<DevicesSummaryResponse, Error>({ | |
| queryKey: ["devices", activeNetwork?.net_name, activeGroup?.grp_title], | |
| queryFn: () => { | |
| if (!activeNetwork?.net_name || !activeGroup?.grp_title) { | |
| throw new Error("Network and group must be selected"); | |
| } | |
| return devices.getDevicesSummary( | |
| activeNetwork?.net_name || "", | |
| activeGroup?.grp_title || "" | |
| ); | |
| }, | |
| enabled: !!activeNetwork?.net_name && !!activeGroup?.grp_title, | |
| retry: (failureCount, error) => { | |
| return failureCount < 3 && !error.message.includes("Network and group must be selected"); | |
| }, | |
| retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), | |
| onSuccess: (data, error) => { | |
| if (error) { | |
| dispatch(setError(error.message)); | |
| } else if (data) { | |
| dispatch(setDevices(data.devices)); | |
| } | |
| }, | |
| }); |
| getSitesSummary: async (networkId: string, groupName: string) => { | ||
| try { | ||
| const response = await axiosInstance.get( | ||
| `${SITES_MGT_URL}/summary?network=${networkId}` | ||
| `${SITES_MGT_URL}/summary?network=${networkId}&group=${groupName}` | ||
| ); | ||
| return response.data; | ||
| } catch (error: any) { | ||
| } catch (error) { | ||
| const axiosError = error as AxiosError<ErrorResponse>; | ||
| throw new Error( | ||
| error.response?.data?.message || "Failed to fetch sites summary" | ||
| axiosError.response?.data?.message || "Failed to fetch sites summary" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add URL parameter encoding for safety.
The URL parameters should be encoded to prevent potential injection attacks.
- `${SITES_MGT_URL}/summary?network=${networkId}&group=${groupName}`
+ `${SITES_MGT_URL}/summary?network=${encodeURIComponent(networkId)}&group=${encodeURIComponent(groupName)}`📝 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.
| getSitesSummary: async (networkId: string, groupName: string) => { | |
| try { | |
| const response = await axiosInstance.get( | |
| `${SITES_MGT_URL}/summary?network=${networkId}` | |
| `${SITES_MGT_URL}/summary?network=${networkId}&group=${groupName}` | |
| ); | |
| return response.data; | |
| } catch (error: any) { | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| error.response?.data?.message || "Failed to fetch sites summary" | |
| axiosError.response?.data?.message || "Failed to fetch sites summary" | |
| ); | |
| } | |
| getSitesSummary: async (networkId: string, groupName: string) => { | |
| try { | |
| const response = await axiosInstance.get( | |
| `${SITES_MGT_URL}/summary?network=${encodeURIComponent(networkId)}&group=${encodeURIComponent(groupName)}` | |
| ); | |
| return response.data; | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| axiosError.response?.data?.message || "Failed to fetch sites summary" | |
| ); | |
| } |
| getApproximateCoordinates: async ( | ||
| latitude: string, | ||
| longitude: string | ||
| ): Promise<ApproximateCoordinatesResponse> => { | ||
| try { | ||
| const response = await axiosInstance.get<ApproximateCoordinatesResponse>( | ||
| `${SITES_MGT_URL}/approximate?latitude=${latitude}&longitude=${longitude}` | ||
| ); | ||
| return response.data; | ||
| } catch (error) { | ||
| const axiosError = error as AxiosError<ErrorResponse>; | ||
| throw new Error( | ||
| axiosError.response?.data?.message || | ||
| "Failed to get approximate coordinates" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add URL parameter encoding for coordinate values.
Similar to the previous issue, coordinate values in the URL should be encoded.
- `${SITES_MGT_URL}/approximate?latitude=${latitude}&longitude=${longitude}`
+ `${SITES_MGT_URL}/approximate?latitude=${encodeURIComponent(latitude)}&longitude=${encodeURIComponent(longitude)}`📝 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.
| getApproximateCoordinates: async ( | |
| latitude: string, | |
| longitude: string | |
| ): Promise<ApproximateCoordinatesResponse> => { | |
| try { | |
| const response = await axiosInstance.get<ApproximateCoordinatesResponse>( | |
| `${SITES_MGT_URL}/approximate?latitude=${latitude}&longitude=${longitude}` | |
| ); | |
| return response.data; | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| axiosError.response?.data?.message || | |
| "Failed to get approximate coordinates" | |
| ); | |
| } | |
| getApproximateCoordinates: async ( | |
| latitude: string, | |
| longitude: string | |
| ): Promise<ApproximateCoordinatesResponse> => { | |
| try { | |
| const response = await axiosInstance.get<ApproximateCoordinatesResponse>( | |
| `${SITES_MGT_URL}/approximate?latitude=${encodeURIComponent(latitude)}&longitude=${encodeURIComponent(longitude)}` | |
| ); | |
| return response.data; | |
| } catch (error) { | |
| const axiosError = error as AxiosError<ErrorResponse>; | |
| throw new Error( | |
| axiosError.response?.data?.message || | |
| "Failed to get approximate coordinates" | |
| ); | |
| } |
| {/* Network Switcher */} | ||
| <div className="mb-4"> | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| className="w-full justify-between text-foreground" | ||
| > | ||
| <div className="flex items-center gap-2"> | ||
| <NetworkIcon size={18} /> | ||
| <span className="uppercase"> | ||
| {activeNetwork?.net_name || "Select Network"} | ||
| </span> | ||
| </div> | ||
| <ChevronDown size={16} /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent className="w-56"> | ||
| <DropdownMenuLabel>Select Network</DropdownMenuLabel> | ||
| <DropdownMenuSeparator /> | ||
| {availableNetworks | ||
| .filter((network) => network._id === activeNetwork?._id) | ||
| .map((network) => ( | ||
| <DropdownMenuItem | ||
| key={network._id} | ||
| onClick={() => handleNetworkChange(network)} | ||
| className="flex items-center justify-between uppercase" | ||
| > | ||
| {network.net_name} | ||
| {activeNetwork?._id === network._id && ( | ||
| <Check size={16} /> | ||
| )} | ||
| </DropdownMenuItem> | ||
| ))} | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| </div> |
There was a problem hiding this comment.
Verify the network filtering logic.
The network filtering logic seems overly restrictive:
.filter((network) => network._id === activeNetwork?._id)This will only show the currently active network, preventing users from switching to other networks.
-availableNetworks
- .filter((network) => network._id === activeNetwork?._id)
+availableNetworks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx (1)
37-39: Consider renaming the Error importThe imported
Errorfrom 'next/error' shadows the globalErrorobject, which could lead to confusion. Consider renaming it to something more specific likeNextError.-import Error from "next/error"; +import NextError from "next/error";🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Do not shadow the global "Error" 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx
[error] 39-39: Do not shadow the global "Error" 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)
[error] 48-48: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 57-57: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
netmanager-app/app/(authenticated)/sites/create-site-form.tsx (2)
144-144: Well-implemented coordinate optimization!The coordinate optimization feature is thoughtfully implemented with:
- Proper loading states and user feedback
- Disabled state handling
- Clean error handling
- Type-safe API integration
Also applies to: 289-333
289-333: Clean and user-friendly UI implementationThe UI changes maintain consistency with the existing design while providing clear feedback through:
- Loading indicators
- Disabled states
- Clear button labels
- Proper spacing and layout
| latitude: z.string().refine( | ||
| (val) => { | ||
| const num = parseFloat(val); | ||
| return !isNaN(num) && num >= -90 && num <= 90; | ||
| }, | ||
| { | ||
| message: "Latitude must be a valid number between -90 and 90", | ||
| } | ||
| ), | ||
| longitude: z.string().refine( | ||
| (val) => { | ||
| const num = parseFloat(val); | ||
| return !isNaN(num) && num >= -180 && num <= 180; | ||
| }, | ||
| { | ||
| message: "Longitude must be a valid number between -180 and 180", | ||
| } | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Number.isNaN for safer number validation
The latitude and longitude validation uses the unsafe global isNaN which performs type coercion. Replace it with Number.isNaN for more predictable validation.
latitude: z.string().refine(
(val) => {
const num = parseFloat(val);
- return !isNaN(num) && num >= -90 && num <= 90;
+ return !Number.isNaN(num) && num >= -90 && num <= 90;
},
{
message: "Latitude must be a valid number between -90 and 90",
}
),
longitude: z.string().refine(
(val) => {
const num = parseFloat(val);
- return !isNaN(num) && num >= -180 && num <= 180;
+ return !Number.isNaN(num) && num >= -180 && num <= 180;
},
{
message: "Longitude must be a valid number between -180 and 180",
}
),📝 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.
| latitude: z.string().refine( | |
| (val) => { | |
| const num = parseFloat(val); | |
| return !isNaN(num) && num >= -90 && num <= 90; | |
| }, | |
| { | |
| message: "Latitude must be a valid number between -90 and 90", | |
| } | |
| ), | |
| longitude: z.string().refine( | |
| (val) => { | |
| const num = parseFloat(val); | |
| return !isNaN(num) && num >= -180 && num <= 180; | |
| }, | |
| { | |
| message: "Longitude must be a valid number between -180 and 180", | |
| } | |
| ), | |
| latitude: z.string().refine( | |
| (val) => { | |
| const num = parseFloat(val); | |
| return !Number.isNaN(num) && num >= -90 && num <= 90; | |
| }, | |
| { | |
| message: "Latitude must be a valid number between -90 and 90", | |
| } | |
| ), | |
| longitude: z.string().refine( | |
| (val) => { | |
| const num = parseFloat(val); | |
| return !Number.isNaN(num) && num >= -180 && num <= 180; | |
| }, | |
| { | |
| message: "Longitude must be a valid number between -180 and 180", | |
| } | |
| ), |
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 57-57: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration