New Netmanager: The Analytics Page and the export-data page #2386
Conversation
|
Warning Rate limit exceeded@danielmarv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive overhaul of the analytics and data management system in the NetManager application. The changes span multiple components, hooks, and Redux slices, focusing on enhancing data visualization, export capabilities, and state management for cohorts, grids, and devices. The implementation introduces new UI components, custom hooks for data fetching, and expanded Redux state management to provide a more robust and flexible analytics experience. 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: 42
🧹 Nitpick comments (46)
netmanager-app/components/Analytics/ExceedeanceLine.tsx.tsx (3)
51-56: Specify explicit types for state variables instead of usingany[]To enhance type safety and leverage TypeScript's strengths, consider defining specific types for
datasetandallLocationsDatainstead of usingany[]. This will help catch errors at compile time and improve code maintainability.Example:
interface ExceedanceData { name: string; [key: string]: number | string; } const [dataset, setDataset] = useState<ExceedanceData[]>([]); const [allLocationsData, setAllLocationsData] = useState<ExceedanceData[]>([]);
90-93: Provide user feedback when data fetching failsCurrently, when an error occurs during data fetching, the error is logged to the console, and the dataset is reset without informing the user. Consider displaying an error message in the UI to notify users that data could not be loaded.
Example:
} catch (error) { console.error('Error fetching data:', error); + setError(true); setDataset([]); }Then, in your render logic, display an error message when
erroris true.{error ? ( <div className="flex justify-center items-center h-64">Failed to load data. Please try again.</div> ) : loading ? ( // existing loading state ) : // rest of the component
170-174: Remove commented-out code to improve code cleanlinessThere's a block of commented-out code related to viewing all exceedances. If this code is no longer needed, consider removing it to keep the codebase clean and maintainable.
netmanager-app/core/hooks/categories.ts (1)
1-8: Consider standardizing category names and range boundaries.A few suggestions to improve the PM2.5 categories:
- The abbreviation "UHFSG" is unclear. Consider using the full name "UnhealthyForSensitiveGroups" or "Unhealthy_For_Sensitive_Groups" for better clarity.
- Consider using whole numbers for range boundaries to avoid potential floating-point comparison issues.
export const PM_25_CATEGORY = { Good: [0, 12], Moderate: [12, 35.4], - UHFSG: [35.4, 55.4], + UnhealthyForSensitiveGroups: [35, 55], - Unhealthy: [55.4, 150.4], + Unhealthy: [55, 150], - VeryUnhealthy: [150.4, 250.4], + VeryUnhealthy: [150, 250], - Hazardous: [250.4, 500.4], + Hazardous: [250, 500], };netmanager-app/app/types/export.ts (1)
5-17: Enhance type definitions with validation constraints and documentation.The FormData interface could benefit from:
- More specific types for datetime fields (consider using
Datetype)- Documentation about valid values for string fields
- Validation constraints using string literals for known values
export interface FormData { - startDateTime: string; - endDateTime: string; + startDateTime: Date; // ISO 8601 format + endDateTime: Date; // ISO 8601 format sites?: Option[]; devices?: Option[]; cities?: Option[]; regions?: Option[]; pollutants?: Option[]; - frequency: string - fileType: string - outputFormat: string - dataType: string + frequency: 'hourly' | 'daily' | 'monthly' | 'yearly'; // Available frequency options + fileType: 'csv' | 'json' | 'xlsx'; // Supported file types + outputFormat: 'raw' | 'processed'; // Output format options + dataType: 'calibrated' | 'uncalibrated'; // Data type options }netmanager-app/core/urls.tsx (1)
9-10: LGTM! Consider adding JSDoc comments.The URL constants are well-structured and follow the established patterns. A small enhancement would be to add JSDoc comments describing the purpose and expected response format of these endpoints.
/** Endpoint for retrieving exceedances data analytics * @returns {Promise<ExceedancesData>} Analytics data for exceedances */ export const EXCEEDANCES_DATA_URI = `${ANALYTICS_MGT_URL}/exceedances`; /** Endpoint for retrieving device-specific exceedances data * @returns {Promise<DeviceExceedances>} Exceedances data per device */ export const DEVICE_EXCEEDANCES_URI = `${ANALYTICS_MGT_URL}/dashboard/exceedances-devices`;netmanager-app/core/redux/store.ts (1)
13-14: Consider alphabetically ordering reducers for better maintainability.The reducer configuration looks good, but organizing the reducers alphabetically would make it easier to locate specific slices as the store grows.
reducer: { + cohorts: cohortsReducer, devices: devicesReducer, + grids: gridsReducer, sites: sitesReducer, user: userReducer, - grids: gridsReducer, - cohorts: cohortsReducer, },netmanager-app/components/ui/toaster.tsx (1)
13-35: Enhance accessibility and add prop documentation.The Toaster implementation looks good but could benefit from accessibility improvements and proper documentation.
+ interface ToastProps { + /** Unique identifier for the toast */ + id: string; + /** Main heading of the toast */ + title?: string; + /** Detailed message of the toast */ + description?: string; + /** Optional action component */ + action?: React.ReactNode; + } export function Toaster() { const { toasts } = useToast() return ( <ToastProvider> {toasts.map(function ({ id, title, description, action, ...props }) { return ( - <Toast key={id} {...props}> + <Toast + key={id} + {...props} + role="alert" + aria-live="polite" + > <div className="grid gap-1"> - {title && <ToastTitle>{title}</ToastTitle>} + {title && <ToastTitle role="heading">{title}</ToastTitle>} {description && ( <ToastDescription>{description}</ToastDescription> )} </div> {action} - <ToastClose /> + <ToastClose aria-label="Close notification" /> </Toast> ) })} <ToastViewport /> </ToastProvider> ) }netmanager-app/core/hooks/useGrids.ts (1)
15-17: Consider adding type safety to the success callbackThe
dataparameter inonSuccessis typed asany. Consider defining an interface for the API response and using it here for better type safety.- onSuccess: (data: any) => { + interface GridsResponse { + grids: Grid[]; + } + onSuccess: (data: GridsResponse) => {netmanager-app/core/apis/cohorts.ts (1)
1-34: Consider implementing request cancellationSince this is for an analytics page where users might navigate away quickly, consider implementing request cancellation using axios cancel tokens to prevent memory leaks and unnecessary network requests.
netmanager-app/core/hooks/useAirqlouds.ts (1)
19-21: Consider error type narrowing for better error handlingThe error handling could benefit from proper type narrowing to handle different types of errors appropriately.
- onError: (error: Error) => { - dispatch(setError(error.message)); - }, + onError: (error: unknown) => { + const errorMessage = error instanceof Error + ? error.message + : 'An unknown error occurred'; + dispatch(setError(errorMessage)); + },netmanager-app/core/redux/slices/deviceSlice.ts (2)
15-19: Fix interface indentationThe
DevicesStateinterface has inconsistent indentation compared to the rest of the file.export interface DevicesState { - devices: Device[]; - isLoading: boolean; - error: string | null; - } + devices: Device[]; + isLoading: boolean; + error: string | null; +}
4-12: Consider adding readonly modifiers to Device interface propertiesSince these properties shouldn't be modified directly, consider adding readonly modifiers for better type safety.
export interface Device { - _id: string; - group: string; - name: string; - authRequired: boolean; - serial_number: string; - api_code: string; - groups: string[]; + readonly _id: string; + readonly group: string; + readonly name: string; + readonly authRequired: boolean; + readonly serial_number: string; + readonly api_code: string; + readonly groups: readonly string[]; }netmanager-app/components/ui/popover.tsx (1)
12-28: Enhance accessibility and documentationConsider adding proper JSDoc documentation and improving accessibility with aria labels.
+/** + * A composable popover component that supports custom positioning and animations. + * @param props - Standard Radix UI Popover.Content props + */ const PopoverContent = React.forwardRef< React.ElementRef<typeof PopoverPrimitive.Content>, React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content> >(({ className, align = "center", sideOffset = 4, ...props }, ref) => ( <PopoverPrimitive.Portal> <PopoverPrimitive.Content ref={ref} align={align} sideOffset={sideOffset} + aria-label={props['aria-label'] || 'Popover content'} className={cn( "z-50 w-72 rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", className )} {...props} /> </PopoverPrimitive.Portal> ))netmanager-app/app/(authenticated)/data-export/page.tsx (2)
21-21: Fix grid columns to match number of tabsThe grid is set to 4 columns but only contains 3 tabs, which might cause uneven spacing.
- <TabsList className="grid w-full grid-cols-4"> + <TabsList className="grid w-full grid-cols-3">
9-33: Consider adding error boundary and loading statesThe component could benefit from proper error handling and loading states for better user experience.
Consider wrapping the component with an error boundary and adding loading states:
import { ErrorBoundary } from '@/components/ErrorBoundary'; import { LoadingSpinner } from '@/components/LoadingSpinner'; export default function ExportData() { const [activeTab, setActiveTab] = useState<ExportType>('sites'); const [isLoading, setIsLoading] = useState(false); return ( <ErrorBoundary fallback={<div>Something went wrong</div>}> <div className="container mx-auto p-4"> {/* ... existing code ... */} <Card> <Tabs value={activeTab} onValueChange={(value) => setActiveTab(value as ExportType)}> {/* ... existing code ... */} <TabsContent value={activeTab}> {isLoading ? ( <LoadingSpinner /> ) : ( <ExportForm exportType={activeTab} onLoadingChange={setIsLoading} /> )} </TabsContent> </Tabs> </Card> </div> </ErrorBoundary> ); }netmanager-app/app/types/grids.ts (1)
40-51: Add JSDoc comments for complex interfacesThe
SiteCategoryinterface contains domain-specific fields that would benefit from documentation explaining their purpose and expected values.Add JSDoc comments to improve code maintainability:
+/** + * Represents the categorization of a site including its geographical and land use attributes. + */ interface SiteCategory { + /** Tags associated with the site category */ tags: string[]; + /** Name of the area where the site is located */ area_name: string; // ... add comments for other fields }netmanager-app/core/redux/slices/gridsSlice.ts (1)
16-21: Consider using discriminated union for error stateThe current state structure doesn't clearly indicate the relationship between
isLoading,error, and data presence.Consider using a discriminated union to make the state more type-safe:
type GridsState = { grids: Grid[]; activeGrid: Grid[] | null; } & ( | { status: 'idle' } | { status: 'loading' } | { status: 'error'; error: string } );netmanager-app/components/ui/date-picker.tsx (1)
22-29: Enhance error handling in handleSelectThe current implementation silently ignores undefined dates. Consider providing feedback to the user when date selection fails.
const handleSelect = (date: Date | undefined) => { if (date) { onChange(date); + } else { + console.warn('Date selection was cancelled or invalid'); + // Optionally: Notify user through UI } }netmanager-app/components/ui/avatar.tsx (1)
8-21: Add size variants to Avatar componentThe Avatar component currently has fixed dimensions. Consider adding size variants for better flexibility.
+const sizes = { + sm: "h-8 w-8", + md: "h-10 w-10", + lg: "h-12 w-12" +} as const; + +type AvatarProps = { + size?: keyof typeof sizes; +} & React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root>; + const Avatar = React.forwardRef< React.ElementRef<typeof AvatarPrimitive.Root>, - React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> ->(({ className, ...props }, ref) => ( + AvatarProps +>(({ className, size = "md", ...props }, ref) => ( <AvatarPrimitive.Root ref={ref} className={cn( - "relative flex h-10 w-10 shrink-0 overflow-hidden rounded-full", + "relative flex shrink-0 overflow-hidden rounded-full", + sizes[size], className )} {...props} /> ))netmanager-app/components/Analytics/AnalyticsDropdown.tsx (2)
19-28: Consider memoizing the handleChange function.Since the function is recreated on every render, consider using useCallback to memoize it for better performance.
- const handleChange = (value: string) => { + const handleChange = React.useCallback((value: string) => { const selectedAirqloud = airqloudsData.find(airqloud => airqloud._id === value) if (selectedAirqloud) { if (isCohort && onSetActiveCohort) { onSetActiveCohort(selectedAirqloud) } else if (onSetActiveGrid) { onSetActiveGrid(selectedAirqloud) } } - } + }, [airqloudsData, isCohort, onSetActiveCohort, onSetActiveGrid])
30-43: Add error handling for empty airqloudsData.The component should handle cases where airqloudsData is empty.
return ( + <> + {airqloudsData.length === 0 ? ( + <p className="text-sm text-gray-500">No data available</p> + ) : ( <Select onValueChange={handleChange}> <SelectTrigger> <SelectValue placeholder={`Select ${isCohort ? 'Cohort' : 'Grid'}`} /> </SelectTrigger> <SelectContent> {airqloudsData.map((airqloud) => ( <SelectItem key={airqloud._id} value={airqloud._id}> {airqloud.name} </SelectItem> ))} </SelectContent> </Select> + )} + </> )netmanager-app/core/redux/slices/analyticsSlice.ts (2)
13-19: Consider enhancing the error state type.The error state could be more detailed to handle different types of errors.
- error: string | null; + error: { + message: string; + code?: string; + details?: unknown; + } | null;
29-52: Add a reset action for state cleanup.Consider adding a reset action to clear the state when navigating away or unmounting components.
reducers: { + reset: () => initialState, setAirqlouds(state, action: PayloadAction<Airqloud[]>) { state.airqlouds = action.payload; state.isLoading = false; state.error = null; }, // ... other reducers },netmanager-app/core/redux/slices/cohortsSlice.ts (1)
41-61: Add reset action for consistency.For consistency with the analytics slice, add a reset action to clear the state.
reducers: { + reset: () => initialState, setCohorts(state, action: PayloadAction<Cohort[]>) { // ... existing code }, // ... other reducers },netmanager-app/components/ui/tabs.tsx (1)
1-55: Consider enhancing keyboard navigation accessibility.The implementation looks solid! Consider adding
aria-labeloraria-descriptionto improve screen reader context, especially for theTabsListcomponent.const TabsList = React.forwardRef< React.ElementRef<typeof TabsPrimitive.List>, React.ComponentPropsWithoutRef<typeof TabsPrimitive.List> >(({ className, ...props }, ref) => ( <TabsPrimitive.List ref={ref} + aria-label="Tab navigation" className={cn( "inline-flex h-10 items-center justify-center rounded-md bg-muted p-1 text-muted-foreground", className )} {...props} /> ))netmanager-app/components/Charts/Bar.tsx (1)
55-62: Add Y-axis for better data interpretation.The chart is missing a Y-axis, which would help users better understand the values.
<CartesianGrid vertical={false} /> +<YAxis + tickLine={false} + tickMargin={10} + axisLine={false} + width={40} +/> <XAxisnetmanager-app/components/Analytics/ExceedanceBar.tsx (1)
47-77: Enhance component robustness and accessibilityConsider these improvements:
- Add error boundaries to handle chart rendering failures
- Implement loading states during data fetching
- Add aria-labels for better accessibility
Example implementation:
export function BarCharts() { const [isLoading, setIsLoading] = useState(true); // ... data fetching logic here if (isLoading) { return <div aria-label="Loading chart data">Loading...</div>; } return ( <ErrorBoundary fallback={<div>Error loading chart</div>}> <div aria-label="Monthly air quality exceedance chart"> {/* existing chart code */} </div> </ErrorBoundary> ); }netmanager-app/components/Analytics/PollutantCategory.tsx (2)
8-19: Add JSDoc comments to improve type documentation.Consider adding JSDoc comments to describe the purpose and usage of these interfaces. This will improve code maintainability and help other developers understand the expected data structure.
+/** + * Represents a site or device with air quality measurements + */ interface Site { label: string pm2_5: number } +/** + * Props for the PollutantCategory component + * @property {string} pm25level - The PM2.5 level category + * @property {string} iconClass - CSS class for the icon + * @property {Site[]} [sites] - Optional array of sites + * @property {Site[]} [devices] - Optional array of devices + */ interface PollutantCategoryProps {
21-77: Optimize performance with useMemo and useCallback.The sorting operation and event handlers could be memoized to prevent unnecessary recalculations.
- const sortedData = [...(sites || devices || [])].sort((a, b) => b.pm2_5 - a.pm2_5) + const sortedData = React.useMemo(() => + [...(sites || devices || [])].sort((a, b) => b.pm2_5 - a.pm2_5), + [sites, devices] + ) - const toggleShow = () => setShow(!show) + const toggleShow = React.useCallback(() => setShow(!show), []) - const handleUnselect = (option: Option) => { + const handleUnselect = React.useCallback((option: Option) => { onChange(selected.filter((s) => s.value !== option.value)) - } + }, [selected, onChange])🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: 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)
netmanager-app/components/ui/multi-select.tsx (1)
19-42: Enhance form integration capabilities.Consider integrating with form libraries and adding validation support:
- Add support for React Hook Form
- Include validation states
- Add aria-invalid and aria-describedby attributes
type MultiSelectProps = { options: Option[] selected: Option[] onChange: (options: Option[]) => void placeholder?: string + error?: string + required?: boolean + name?: string + 'aria-describedby'?: string } export function MultiSelect({ options, selected = [], onChange, - placeholder = "Select items..." + placeholder = "Select items...", + error, + required, + name, + 'aria-describedby': ariaDescribedby }: MultiSelectProps) {netmanager-app/components/ui/use-toast.ts (1)
73-126: Improve state management implementation.
- Move side effects out of the reducer
- Enhance type safety for actions
- Consider using a more robust state management solution
+const ACTIONS = { + ADD_TOAST: 'ADD_TOAST', + UPDATE_TOAST: 'UPDATE_TOAST', + DISMISS_TOAST: 'DISMISS_TOAST', + REMOVE_TOAST: 'REMOVE_TOAST' +} as const -type ActionType = { - ADD_TOAST: "ADD_TOAST", - UPDATE_TOAST: "UPDATE_TOAST", - DISMISS_TOAST: "DISMISS_TOAST", - REMOVE_TOAST: "REMOVE_TOAST", -} +type ActionType = typeof ACTIONS +// Move side effects to a separate function +function handleToastDismiss(toastId?: string) { + if (toastId) { + addToRemoveQueue(toastId) + } else { + memoryState.toasts.forEach((toast) => { + addToRemoveQueue(toast.id) + }) + } +} export const reducer = (state: State, action: Action): State => { switch (action.type) { case "DISMISS_TOAST": { - // Side effects removed from reducer + handleToastDismiss(action.toastId)netmanager-app/components/Analytics/CohortsDashboard.tsx (3)
22-22: Remove commented out code.Commented code should be removed to maintain code cleanliness.
- // const [chartTypePM, setChartTypePM] = useState<'line' | 'bar'>('bar')
24-31: Consider moving categories to a constants file.The PM2.5 level categories could be moved to a separate constants file for better reusability across components.
Create a new file
constants/pollutants.ts:export const PM25_CATEGORIES = [ { pm25level: "Good", iconClass: "bg-green-500" }, { pm25level: "Moderate", iconClass: "bg-yellow-500" }, { pm25level: "Unhealthy for Sensitive Groups", iconClass: "bg-orange-500" }, { pm25level: "Unhealthy", iconClass: "bg-red-500" }, { pm25level: "Very Unhealthy", iconClass: "bg-purple-500" }, { pm25level: "Hazardous", iconClass: "bg-rose-900" } ]
98-103: Consider using discriminated unions instead of boolean flags.Using boolean flags
isGridsandisCohortssuggests mutually exclusive states that could be better represented using a discriminated union type.type AnalyticsType = { type: 'grid' | 'cohort'; analyticsSites?: any[]; analyticsDevices?: any[]; }; <ExceedancesChart {...{ type: 'cohort', analyticsDevices: activeCohort?.devices || [] }} />netmanager-app/components/ui/toast.tsx (1)
82-82: Use data- prefix for custom attributes.Custom attributes should follow HTML5 specifications by using the data- prefix.
- toast-close="" + data-toast-close=""netmanager-app/components/ui/command.tsx (1)
30-30: Consider using Tailwind's @apply for complex class strings.The long class string could be simplified using Tailwind's @apply directive in a separate CSS file.
/* styles/command.css */ .command-dialog { @apply [&_[cmdk-group-heading]]:px-2 [&_[cmdk-group-heading]]:font-medium [&_[cmdk-group-heading]]:text-muted-foreground [&_[cmdk-group]:not([hidden])_~[cmdk-group]]:pt-0 [&_[cmdk-group]]:px-2; }netmanager-app/components/Analytics/index.tsx (2)
65-69: Extract configuration values to constants.Date range, frequency, and pollutants are hard-coded. Consider making these configurable.
const ANALYTICS_CONFIG = { defaultDateRange: 5, // days defaultFrequency: 'hourly', defaultPollutants: ['pm2_5', 'pm10'] } as const;
76-82: Improve error handling with specific error messages.The error handling could be more informative by providing specific error messages based on the error type.
} catch (error) { const errorMessage = error instanceof Error ? error.message : "An unexpected error occurred while downloading data"; toast({ title: "Error", description: errorMessage, variant: "destructive", }); }netmanager-app/components/ui/select.tsx (2)
15-32: Add aria-label to SelectTrigger for better accessibility.The SelectTrigger component should include an aria-label prop to improve accessibility for screen readers.
const SelectTrigger = React.forwardRef< React.ElementRef<typeof SelectPrimitive.Trigger>, React.ComponentPropsWithoutRef<typeof SelectPrimitive.Trigger> >(({ className, children, ...props }, ref) => ( <SelectPrimitive.Trigger ref={ref} + aria-label="Select option" className={cn(
1-160: Consider adding prop types documentation and error boundaries.The component could benefit from:
- JSDoc documentation for the exported components and their props
- Error boundary wrapper to handle potential runtime errors gracefully
netmanager-app/components/Charts/Pie.tsx (2)
26-32: Move hard-coded data to a separate configuration file.The
desktopDataarray should be moved to a separate configuration file or fetched from an API to improve maintainability.
125-182: Enhance chart accessibility and add loading state.The pie chart implementation could be improved by:
- Adding ARIA labels for chart elements
- Implementing a loading state
- Including keyboard navigation support for chart interactions
Example implementation for loading state:
<PieChart> + {loading ? ( + <text x="50%" y="50%" textAnchor="middle" dominantBaseline="middle"> + Loading... + </text> + ) : ( <ChartTooltip cursor={false} content={<ChartTooltipContent hideLabel />} /> // ... rest of the chart implementation + )} </PieChart>netmanager-app/package.json (2)
28-28: Consider standardizing on a single charting library.I notice we're adding both
apexcharts(withreact-apexcharts) andrecharts. While both are excellent libraries, maintaining multiple charting libraries could lead to:
- Inconsistent visualization styles across the application
- Increased bundle size
- Higher maintenance overhead
Consider standardizing on one library based on your specific needs:
rechartsis great for simpler, React-native chartsapexchartsoffers more complex visualizations but has a larger footprintAlso applies to: 40-40, 47-47
32-32: Consider consolidating date-related dependencies.The addition of both
date-fnsandreact-day-pickermight introduce overlap in date handling functionality. Whilereact-day-pickerusesdate-fnsinternally, ensure you're leveraging each library for its intended purpose:
date-fnsfor date manipulationreact-day-pickerspecifically for calendar UI componentsAlso applies to: 33-33, 41-41
netmanager-app/app/globals.css (1)
32-35: Consider documenting color usage patterns.While the color implementation is solid, it would be helpful to add a comment block explaining:
- The intended use case for each chart color
- Any specific data type or chart type associations
- Color accessibility considerations
Example documentation:
/* Chart Colors Guide * --chart-6: Blue - Primary metric * --chart-7: Red - Alert/Critical metrics * --chart-8: Green - Success/Positive metrics * --chart-9: Yellow - Warning/Attention metrics * * All colors meet WCAG 2.1 AA contrast requirements */Also applies to: 63-66
📜 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 (51)
netmanager-app/app/(authenticated)/analytics/page.tsx(1 hunks)netmanager-app/app/(authenticated)/data-export/page.tsx(1 hunks)netmanager-app/app/globals.css(2 hunks)netmanager-app/app/types/cohorts.ts(1 hunks)netmanager-app/app/types/export.ts(1 hunks)netmanager-app/app/types/grids.ts(1 hunks)netmanager-app/components/Analytics/AirqloudDropdown.tsx(1 hunks)netmanager-app/components/Analytics/AnalyticsDropdown.tsx(1 hunks)netmanager-app/components/Analytics/CohortsDashboard.tsx(1 hunks)netmanager-app/components/Analytics/ExceedanceBar.tsx(1 hunks)netmanager-app/components/Analytics/ExceedeanceLine.tsx.tsx(1 hunks)netmanager-app/components/Analytics/GridDashboard.tsx(1 hunks)netmanager-app/components/Analytics/PollutantCategory.tsx(1 hunks)netmanager-app/components/Analytics/index.tsx(1 hunks)netmanager-app/components/Charts/Bar.tsx(1 hunks)netmanager-app/components/Charts/Line.tsx(1 hunks)netmanager-app/components/Charts/PMchart.tsx(1 hunks)netmanager-app/components/Charts/Pie.tsx(1 hunks)netmanager-app/components/Charts/Speed.tsx(1 hunks)netmanager-app/components/export-data/ExportForm.tsx(1 hunks)netmanager-app/components/ui/ErrorBoundory.tsx(1 hunks)netmanager-app/components/ui/avatar.tsx(1 hunks)netmanager-app/components/ui/calendar.tsx(1 hunks)netmanager-app/components/ui/card.tsx(6 hunks)netmanager-app/components/ui/chart.tsx(1 hunks)netmanager-app/components/ui/command.tsx(1 hunks)netmanager-app/components/ui/date-picker.tsx(1 hunks)netmanager-app/components/ui/multi-select.tsx(1 hunks)netmanager-app/components/ui/popover.tsx(1 hunks)netmanager-app/components/ui/select.tsx(1 hunks)netmanager-app/components/ui/skeleton.tsx(1 hunks)netmanager-app/components/ui/sonner.tsx(2 hunks)netmanager-app/components/ui/tabs.tsx(1 hunks)netmanager-app/components/ui/toast.tsx(1 hunks)netmanager-app/components/ui/toaster.tsx(1 hunks)netmanager-app/components/ui/use-toast.ts(1 hunks)netmanager-app/core/apis/analytics.ts(1 hunks)netmanager-app/core/apis/cohorts.ts(1 hunks)netmanager-app/core/apis/grids.ts(1 hunks)netmanager-app/core/hooks/categories.ts(1 hunks)netmanager-app/core/hooks/useAirqlouds.ts(1 hunks)netmanager-app/core/hooks/useCohorts.ts(1 hunks)netmanager-app/core/hooks/useGrids.ts(1 hunks)netmanager-app/core/redux/slices/analyticsSlice.ts(1 hunks)netmanager-app/core/redux/slices/cohortsSlice.ts(1 hunks)netmanager-app/core/redux/slices/deviceSlice.ts(1 hunks)netmanager-app/core/redux/slices/gridsSlice.ts(1 hunks)netmanager-app/core/redux/store.ts(1 hunks)netmanager-app/core/urls.tsx(1 hunks)netmanager-app/package.json(1 hunks)netmanager-app/tailwind.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- netmanager-app/components/ui/sonner.tsx
- netmanager-app/components/ui/card.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/ui/chart.tsx
[error] 81-81: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
netmanager-app/components/Analytics/PollutantCategory.tsx
[error] 67-67: 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 (10)
netmanager-app/app/(authenticated)/analytics/page.tsx (1)
1-7: Code looks good and simplifies the analytics pageThe update to import and render the new
Analyticscomponent streamlines the page and improves code organization.netmanager-app/components/ui/skeleton.tsx (1)
3-13: Clean and well-implemented skeleton component! 👍The implementation is type-safe, reusable, and follows React best practices. Good use of the utility function for class name merging.
netmanager-app/components/Analytics/AnalyticsDropdown.tsx (1)
6-11: LGTM! Well-structured interface with clear type definitions.The props interface is well-defined with proper TypeScript types and optional callbacks.
netmanager-app/tailwind.config.ts (1)
52-55: LGTM! Chart color palette extension looks good.The new chart colors (6-9) follow the established pattern and maintain consistency with the existing color system.
netmanager-app/components/ui/calendar.tsx (1)
1-70: Well-implemented calendar component!The calendar implementation is robust with:
- Good use of utility functions
- Comprehensive styling configuration
- Proper accessibility considerations
- Clean prop handling
netmanager-app/components/Analytics/CohortsDashboard.tsx (1)
1-18: LGTM! Props and imports are well structured.The component's interface is well-defined with appropriate prop types, and imports are organized logically.
netmanager-app/components/ui/toast.tsx (1)
25-40: LGTM! Toast variants are well structured.The toast variants are well-defined using class-variance-authority with appropriate styling for different states.
netmanager-app/components/Analytics/GridDashboard.tsx (1)
58-95: 🛠️ Refactor suggestionAdd cleanup function to useEffect to prevent memory leaks.
The useEffect hook should include a cleanup function to prevent potential memory leaks when the component unmounts.
useEffect(() => { + let isSubscribed = true; if (!activeGrid || !recentEventsData?.features) return; // ... existing code ... - setPm2_5SiteCount(initialCount); + if (isSubscribed) { + setPm2_5SiteCount(initialCount); + } + + return () => { + isSubscribed = false; + }; }, [activeGrid, recentEventsData]);Likely invalid or redundant comment.
netmanager-app/package.json (1)
13-13: Verify Radix UI version compatibility.The new Radix UI components (
@radix-ui/react-avatar,@radix-ui/react-popover,@radix-ui/react-select) are using caret versions. While this allows for minor version updates, it could potentially introduce breaking changes.Run this script to check for any known issues or breaking changes:
Also applies to: 19-19, 20-20
✅ Verification successful
The caret versions are safe to use with these Radix UI components
The specified versions with caret ranges are actually beneficial here as they:
- Prevent automatic major version updates that could introduce breaking changes
- Allow patch-level updates for bug fixes
- Follow semantic versioning strictly, ensuring stability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in recent Radix UI releases gh api graphql -f query=' { repository(owner: "radix-ui", name: "primitives") { releases(first: 5, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { tagName description } } } }'Length of output: 276
Script:
#!/bin/bash # Check npm registry for package information and search for changelogs echo "=== NPM Registry Info ===" npm view @radix-ui/react-avatar versions --json npm view @radix-ui/react-popover versions --json npm view @radix-ui/react-select versions --json echo -e "\n=== Searching for changelogs ===" fd -i changelog.mdLength of output: 10466
netmanager-app/app/globals.css (1)
32-35: Well-structured color system for charts! 👍The new chart colors are well-implemented with:
- Consistent HSL format
- Appropriate contrast adjustments for dark mode
- Semantic naming convention that aligns with existing variables
This will help maintain visual consistency across the analytics dashboard.
Also applies to: 63-66
| @@ -0,0 +1,271 @@ | |||
| "use client" | |||
There was a problem hiding this comment.
Correct the filename to maintain consistency
The file name ExceedeanceLine.tsx.tsx appears to have a typo and an extra .tsx extension. It should likely be ExceedancesLine.tsx to adhere to naming conventions and prevent potential import issues.
| const location = isCohorts ? item.device_id : (item.site.name || item.site.description || item.site.generated_name); | ||
| const exceedances = isCohorts ? item.exceedances : item.exceedance; |
There was a problem hiding this comment.
Ensure safe access to item.site to prevent runtime errors
When accessing item.site.name, item.site.description, or item.site.generated_name, ensure that item.site is defined to prevent potential runtime errors if item.site is undefined.
Apply optional chaining or null checks:
- const location = isCohorts ? item.device_id : (item.site.name || item.site.description || item.site.generated_name);
+ const location = isCohorts ? item.device_id : (item.site?.name || item.site?.description || item.site?.generated_name || 'Unknown');Similarly, in the render method:
- ? location.device_id
- : location.site.name || location.site.description || location.site.generated_name
+ ? location.device_id
+ : location.site?.name || location.site?.description || location.site?.generated_name || 'Unknown'Also applies to: 238-240
|
|
||
| return ( | ||
| <style | ||
| dangerouslySetInnerHTML={{ |
There was a problem hiding this comment.
Avoid using dangerouslySetInnerHTML to prevent XSS vulnerabilities
The use of dangerouslySetInnerHTML at line 81 can expose the application to cross-site scripting (XSS) attacks. Even though the content is generated internally, it's safer to avoid using dangerouslySetInnerHTML whenever possible.
Consider using a safer alternative, such as generating the styles using a CSS-in-JS library or setting styles via React's style prop.
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| export const dataExport = async (data: FormData) => { | ||
|
|
||
| const headers = { | ||
| service: 'data-export' | ||
| } | ||
| return axiosInstance.post<{ downloadUrl: string }>( | ||
| `${ANALYTICS_MGT_URL}/data-download`, | ||
| data, | ||
| { headers } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety.
The API implementation could be enhanced with:
- Proper error handling
- More specific response type definition
- Error retry logic for network issues
+interface ExportResponse {
+ downloadUrl: string;
+ expiresAt?: string;
+}
+
-export const dataExport = async (data: FormData) => {
+export const dataExport = async (data: FormData): Promise<ExportResponse> => {
+ try {
const headers = {
service: 'data-export'
}
- return axiosInstance.post<{ downloadUrl: string }>(
+ const response = await axiosInstance.post<ExportResponse>(
`${ANALYTICS_MGT_URL}/data-download`,
data,
{ headers }
);
+ return response.data;
+ } catch (error) {
+ if (axios.isAxiosError(error)) {
+ throw new Error(`Export failed: ${error.response?.data?.message || error.message}`);
+ }
+ throw error;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| import React, { Component, ErrorInfo, ReactNode } from 'react' | ||
|
|
||
| interface Props { | ||
| children: ReactNode; | ||
| } | ||
|
|
||
| interface State { | ||
| hasError: boolean; | ||
| } | ||
|
|
||
| class ErrorBoundary extends Component<Props, State> { | ||
| public state: State = { | ||
| hasError: false | ||
| }; | ||
|
|
||
| public static getDerivedStateFromError(): State { | ||
| return { hasError: true }; | ||
| } | ||
|
|
||
| public componentDidCatch(error: Error, errorInfo: ErrorInfo) { | ||
| console.error("Uncaught error:", error, errorInfo); | ||
| } | ||
|
|
||
| public render() { | ||
| if (this.state.hasError) { | ||
| return <h1>Sorry.. there was an error</h1>; | ||
| } | ||
|
|
||
| return this.props.children; | ||
| } | ||
| } | ||
|
|
||
| export default ErrorBoundary; | ||
|
|
There was a problem hiding this comment.
Fix filename typo and enhance error handling.
The filename contains a typo: "Boundory" should be "Boundary". Additionally, the error handling implementation could be improved.
- Rename the file from
ErrorBoundory.tsxtoErrorBoundary.tsx - Enhance the error UI and logging:
- console.error("Uncaught error:", error, errorInfo);
+ console.error("Error Boundary caught an error:", {
+ error: error.message,
+ stack: error.stack,
+ componentStack: errorInfo.componentStack
+ });
- return <h1>Sorry.. there was an error</h1>;
+ return (
+ <div className="error-boundary-fallback">
+ <h2>Something went wrong</h2>
+ <details>
+ <summary>Error Details</summary>
+ <pre>{error.message}</pre>
+ </details>
+ <button onClick={() => window.location.reload()}>
+ Refresh Page
+ </button>
+ </div>
+ );Committable suggestion skipped: line range outside the PR's diff.
| export function LineCharts() { | ||
| return ( | ||
| <div> | ||
| <ChartContainer config={chartConfig}> | ||
| <LineChart | ||
| width={500} | ||
| height={200} | ||
| accessibilityLayer | ||
| data={chartData} | ||
| margin={{ | ||
| left: 12, | ||
| right: 12, | ||
| }} | ||
| > | ||
| <CartesianGrid vertical={false} /> | ||
| <XAxis | ||
| dataKey="month" | ||
| tickLine={false} | ||
| axisLine={false} | ||
| tickMargin={8} | ||
| tickFormatter={(value) => value.slice(0, 3)} | ||
| /> | ||
| <ChartTooltip cursor={false} content={<ChartTooltipContent />} /> | ||
| <Line | ||
| dataKey="good" | ||
| type="monotone" | ||
| stroke="#45e50d" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| <Line | ||
| dataKey="moderate" | ||
| type="monotone" | ||
| stroke="#f8fe28" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| <Line | ||
| dataKey="uhfsg" | ||
| type="monotone" | ||
| stroke="#ee8310" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| <Line | ||
| dataKey="unhealthy" | ||
| type="monotone" | ||
| stroke="#fe0000" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| <Line | ||
| dataKey="veryunhealthy" | ||
| type="monotone" | ||
| stroke="#808080" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| <Line | ||
| dataKey="hazadrous" | ||
| type="monotone" | ||
| stroke="#81202e" | ||
| strokeWidth={2} | ||
| dot={false} | ||
| /> | ||
| </LineChart> | ||
| </ChartContainer> | ||
| </div> | ||
|
|
||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance chart component with responsive design and error handling.
The chart component needs improvements in several areas:
- Replace fixed dimensions with responsive values
- Add proper ARIA labels for accessibility
- Implement loading and error states
+interface LineChartsProps {
+ width?: number
+ height?: number
+ loading?: boolean
+ error?: Error
+}
-export function LineCharts() {
+export function LineCharts({
+ width = 500,
+ height = 200,
+ loading,
+ error
+}: LineChartsProps) {
+ if (loading) return <div>Loading chart data...</div>
+ if (error) return <div>Error loading chart: {error.message}</div>
return (
<div>
<ChartContainer config={chartConfig}>
<LineChart
- width={500}
- height={200}
+ width={width}
+ height={height}
accessibilityLayer
+ aria-label="Monthly air quality metrics"
data={chartData}📝 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.
| export function LineCharts() { | |
| return ( | |
| <div> | |
| <ChartContainer config={chartConfig}> | |
| <LineChart | |
| width={500} | |
| height={200} | |
| accessibilityLayer | |
| data={chartData} | |
| margin={{ | |
| left: 12, | |
| right: 12, | |
| }} | |
| > | |
| <CartesianGrid vertical={false} /> | |
| <XAxis | |
| dataKey="month" | |
| tickLine={false} | |
| axisLine={false} | |
| tickMargin={8} | |
| tickFormatter={(value) => value.slice(0, 3)} | |
| /> | |
| <ChartTooltip cursor={false} content={<ChartTooltipContent />} /> | |
| <Line | |
| dataKey="good" | |
| type="monotone" | |
| stroke="#45e50d" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="moderate" | |
| type="monotone" | |
| stroke="#f8fe28" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="uhfsg" | |
| type="monotone" | |
| stroke="#ee8310" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="unhealthy" | |
| type="monotone" | |
| stroke="#fe0000" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="veryunhealthy" | |
| type="monotone" | |
| stroke="#808080" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="hazadrous" | |
| type="monotone" | |
| stroke="#81202e" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| </LineChart> | |
| </ChartContainer> | |
| </div> | |
| ) | |
| } | |
| interface LineChartsProps { | |
| width?: number | |
| height?: number | |
| loading?: boolean | |
| error?: Error | |
| } | |
| export function LineCharts({ | |
| width = 500, | |
| height = 200, | |
| loading, | |
| error | |
| }: LineChartsProps) { | |
| if (loading) return <div>Loading chart data...</div> | |
| if (error) return <div>Error loading chart: {error.message}</div> | |
| return ( | |
| <div> | |
| <ChartContainer config={chartConfig}> | |
| <LineChart | |
| width={width} | |
| height={height} | |
| accessibilityLayer | |
| aria-label="Monthly air quality metrics" | |
| data={chartData} | |
| margin={{ | |
| left: 12, | |
| right: 12, | |
| }} | |
| > | |
| <CartesianGrid vertical={false} /> | |
| <XAxis | |
| dataKey="month" | |
| tickLine={false} | |
| axisLine={false} | |
| tickMargin={8} | |
| tickFormatter={(value) => value.slice(0, 3)} | |
| /> | |
| <ChartTooltip cursor={false} content={<ChartTooltipContent />} /> | |
| <Line | |
| dataKey="good" | |
| type="monotone" | |
| stroke="#45e50d" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="moderate" | |
| type="monotone" | |
| stroke="#f8fe28" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="uhfsg" | |
| type="monotone" | |
| stroke="#ee8310" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="unhealthy" | |
| type="monotone" | |
| stroke="#fe0000" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="veryunhealthy" | |
| type="monotone" | |
| stroke="#808080" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| <Line | |
| dataKey="hazadrous" | |
| type="monotone" | |
| stroke="#81202e" | |
| strokeWidth={2} | |
| dot={false} | |
| /> | |
| </LineChart> | |
| </ChartContainer> | |
| </div> | |
| ) | |
| } |
| const chartData = [ | ||
| { month: "January", good: 186, moderate: 80, uhfsg: 200, unhealthy: 100, veryunhealthy: 150, hazadrous: 50 }, | ||
| { month: "February", good: 305, moderate: 200, uhfsg: 100, unhealthy: 80, veryunhealthy: 50, hazadrous: 40 }, | ||
| { month: "March", good: 237, moderate: 120, uhfsg: 300, unhealthy: 150, veryunhealthy: 60, hazadrous: 100 }, | ||
| { month: "April", good: 73, moderate: 190, uhfsg: 350, unhealthy: 100, veryunhealthy: 50, hazadrous: 200 }, | ||
| { month: "May", good: 209, moderate: 130, uhfsg: 170, unhealthy: 46, veryunhealthy: 70, hazadrous: 30 }, | ||
| { month: "June", good: 214, moderate: 140, uhfsg: 200, unhealthy: 100, veryunhealthy: 100, hazadrous: 50 }, | ||
| { month: "July", good: 186, moderate: 80, uhfsg: 200, unhealthy: 40, veryunhealthy: 70, hazadrous: 150 }, | ||
| { month: "August", good: 305, moderate: 200, uhfsg: 100, unhealthy: 350, veryunhealthy: 50, hazadrous: 100 }, | ||
| { month: "September", good: 237, moderate: 120, uhfsg: 230, unhealthy: 96, veryunhealthy: 89, hazadrous: 200 }, | ||
| { month: "October", good: 73, moderate: 190, uhfsg: 350, unhealthy: 100, veryunhealthy: 250, hazadrous: 95 }, | ||
| { month: "November", good: 209, moderate: 130, uhfsg: 76, unhealthy: 40, veryunhealthy: 200, hazadrous: 50 }, | ||
| { month: "December", good: 214, moderate: 140, uhfsg: 40, unhealthy: 200, veryunhealthy: 50, hazadrous: 170 }, | ||
| ] | ||
|
|
||
| const chartConfig = { | ||
| good: { | ||
| label: "good", | ||
| color: "hsl(var(--chart-6))", | ||
| }, | ||
| moderate: { | ||
| label: "moderate", | ||
| color: "hsl(var(--chart-7))", | ||
| }, | ||
| uhfsg: { | ||
| label: "uhfsg", | ||
| color: "hsl(var(--chart-6))", | ||
| }, | ||
| unhealthy: { | ||
| label: "unhealthy", | ||
| color: "hsl(var(--chart-7))", | ||
| }, | ||
| veryunhealty: { | ||
| label: "veryunhealty", | ||
| color: "hsl(var(--chart-6))", | ||
| }, | ||
| hazadrous: { | ||
| label: "hazadrous", | ||
| color: "hsl(var(--chart-6))", | ||
| }, | ||
| } satisfies ChartConfig |
There was a problem hiding this comment.
Multiple issues in chart data and configuration.
- Hard-coded data should be moved to a separate data source or fetched from an API
- Spelling inconsistency: "hazadrous" vs "hazardous"
- Duplicate color values in chartConfig
-const chartData = [
- { month: "January", good: 186, moderate: 80, uhfsg: 200, unhealthy: 100, veryunhealthy: 150, hazadrous: 50 },
+// Move to a separate file or fetch from API
+type ChartDataPoint = {
+ month: string
+ good: number
+ moderate: number
+ uhfsg: number
+ unhealthy: number
+ veryunhealthy: number
+ hazardous: number // Fix spelling
+}
-const chartConfig = {
+const chartConfig: ChartConfig = {
good: {
label: "good",
- color: "hsl(var(--chart-6))",
+ color: "hsl(var(--chart-green))",
},
// ... other configs with unique colors
hazardous: { // Fix spelling
label: "hazardous",
color: "hsl(var(--chart-red))",
},
}Committable suggestion skipped: line range outside the PR's diff.
| const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| const input = inputRef.current | ||
| if (input) { | ||
| if (e.key === "Delete" || e.key === "Backspace") { | ||
| if (input.value === "") { | ||
| const newSelected = [...selected] | ||
| newSelected.pop() | ||
| onChange(newSelected) | ||
| } | ||
| } | ||
| if (e.key === "Escape") { | ||
| input.blur() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keyboard navigation support.
Add support for additional keyboard interactions to improve accessibility:
- Arrow keys for navigation
- Enter key for selection
- Space key for toggling dropdown
const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
const input = inputRef.current
if (input) {
+ if (e.key === "ArrowDown" || e.key === "ArrowUp") {
+ e.preventDefault()
+ setOpen(true)
+ // Implement focus management
+ }
+ if (e.key === "Enter" && open) {
+ e.preventDefault()
+ // Implement selection of focused option
+ }
+ if (e.key === " " && !input.value) {
+ e.preventDefault()
+ setOpen(!open)
+ }
if (e.key === "Delete" || e.key === "Backspace") {Committable suggestion skipped: line range outside the PR's diff.
| const TOAST_LIMIT = 1 | ||
| const TOAST_REMOVE_DELAY = 1000000 | ||
|
|
||
| type ToasterToast = ToastProps & { | ||
| id: string | ||
| title?: React.ReactNode | ||
| description?: React.ReactNode | ||
| action?: ToastActionElement | ||
| } | ||
|
|
||
| type ActionType = { | ||
| ADD_TOAST: "ADD_TOAST", | ||
| UPDATE_TOAST: "UPDATE_TOAST", | ||
| DISMISS_TOAST: "DISMISS_TOAST", | ||
| REMOVE_TOAST: "REMOVE_TOAST", | ||
| } | ||
|
|
||
| let count = 0 | ||
|
|
||
| function genId() { | ||
| count = (count + 1) % Number.MAX_SAFE_INTEGER | ||
| return count.toString() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust toast configuration values.
The current configuration values seem unusual:
- TOAST_REMOVE_DELAY is set to 16.67 minutes (1000000ms)
- TOAST_LIMIT is set to only 1 toast
Consider using more standard values:
-const TOAST_LIMIT = 1
-const TOAST_REMOVE_DELAY = 1000000
+const TOAST_LIMIT = 3 // Allow multiple toasts
+const TOAST_REMOVE_DELAY = 5000 // 5 seconds is more standard📝 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 TOAST_LIMIT = 1 | |
| const TOAST_REMOVE_DELAY = 1000000 | |
| type ToasterToast = ToastProps & { | |
| id: string | |
| title?: React.ReactNode | |
| description?: React.ReactNode | |
| action?: ToastActionElement | |
| } | |
| type ActionType = { | |
| ADD_TOAST: "ADD_TOAST", | |
| UPDATE_TOAST: "UPDATE_TOAST", | |
| DISMISS_TOAST: "DISMISS_TOAST", | |
| REMOVE_TOAST: "REMOVE_TOAST", | |
| } | |
| let count = 0 | |
| function genId() { | |
| count = (count + 1) % Number.MAX_SAFE_INTEGER | |
| return count.toString() | |
| } | |
| const TOAST_LIMIT = 3 // Allow multiple toasts | |
| const TOAST_REMOVE_DELAY = 5000 // 5 seconds is more standard | |
| type ToasterToast = ToastProps & { | |
| id: string | |
| title?: React.ReactNode | |
| description?: React.ReactNode | |
| action?: ToastActionElement | |
| } | |
| type ActionType = { | |
| ADD_TOAST: "ADD_TOAST", | |
| UPDATE_TOAST: "UPDATE_TOAST", | |
| DISMISS_TOAST: "DISMISS_TOAST", | |
| REMOVE_TOAST: "REMOVE_TOAST", | |
| } | |
| let count = 0 | |
| function genId() { | |
| count = (count + 1) % Number.MAX_SAFE_INTEGER | |
| return count.toString() | |
| } |
| const categorizeSite = (site: Site, pm2_5: number, categories: any) => { | ||
| Object.keys(PM_25_CATEGORY).forEach((key) => { | ||
| const [min, max] = PM_25_CATEGORY[key]; | ||
| if (pm2_5 > min && pm2_5 <= max) { | ||
| categories[key].push({ ...site, pm2_5 }); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type with proper interface.
The categories parameter in categorizeSite function uses the any type, which should be replaced with a proper interface.
- const categorizeSite = (site: Site, pm2_5: number, categories: any) => {
+ interface Categories {
+ [key: string]: Site[];
+ }
+ const categorizeSite = (site: Site, pm2_5: number, categories: Categories) => {📝 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 categorizeSite = (site: Site, pm2_5: number, categories: any) => { | |
| Object.keys(PM_25_CATEGORY).forEach((key) => { | |
| const [min, max] = PM_25_CATEGORY[key]; | |
| if (pm2_5 > min && pm2_5 <= max) { | |
| categories[key].push({ ...site, pm2_5 }); | |
| } | |
| }); | |
| }; | |
| interface Categories { | |
| [key: string]: Site[]; | |
| } | |
| const categorizeSite = (site: Site, pm2_5: number, categories: Categories) => { | |
| Object.keys(PM_25_CATEGORY).forEach((key) => { | |
| const [min, max] = PM_25_CATEGORY[key]; | |
| if (pm2_5 > min && pm2_5 <= max) { | |
| categories[key].push({ ...site, pm2_5 }); | |
| } | |
| }); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
netmanager-app/components/Analytics/AnalyticsDropdown.tsx (4)
3-3: Remove unused importuseState.The
useStatehook is imported but never used in the component.-import React, { useState } from 'react' +import React from 'react'
31-38: Consider optimizing string formatting.The
formatStringfunction could benefit from the following improvements:
- Memoization to prevent unnecessary re-computations
- Combining regex operations for better performance
+const memoizedFormatString = React.useMemo(() => { + const formatCache = new Map<string, string>(); + + return (string: string) => { + if (formatCache.has(string)) { + return formatCache.get(string)!; + } + + const formatted = string + .replace(/(_|\b\w)/g, (char, index) => + char === '_' ? ' ' : index === 0 ? char.toUpperCase() : char.toLowerCase() + ) + .replace(/\bid\b/gi, 'ID'); + + formatCache.set(string, formatted); + return formatted; + }; +}, []);
63-69: Simplify the nested conditional logic for count display.The nested ternary operator makes the code harder to read. Consider extracting this logic into a separate function.
+const getItemCount = (airqloud: Cohort | Grid, isCohort: boolean) => { + if (isCohort && 'devices' in airqloud) { + return `${airqloud.devices?.length || 0} devices`; + } + if (!isCohort && 'sites' in airqloud) { + return `${airqloud.sites?.length || 0} sites`; + } + return ''; +}; -<span className=" text-sm text-gray-500 text-right"> - {isCohort - ? 'devices' in airqloud - ? `${airqloud.devices?.length || 0} devices` - : '' - : 'sites' in airqloud ? `${airqloud.sites?.length || 0} sites` : ''} -</span> +<span className="text-sm text-gray-500 text-right"> + {getItemCount(airqloud, isCohort)} +</span>
58-71: Consider using CSS Grid areas for better maintainability.The current grid implementation uses hardcoded column spans. Using CSS Grid areas would make the layout more maintainable and semantic.
-<div className="grid grid-cols-12 gap-2 w-full items-center"> - <div className="col-span-9"> - <span className="font-medium truncate">{formatString(airqloud.name)}</span> - </div> - <div className="col-span-3 text-sm text-gray-500 text-right"> +<div className="grid grid-areas-[name_count] grid-cols-[1fr_auto] gap-2 w-full items-center"> + <div className="grid-in-name"> + <span className="font-medium truncate block">{formatString(airqloud.name)}</span> + </div> + <div className="grid-in-count">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/Analytics/AnalyticsDropdown.tsx(1 hunks)
🔇 Additional comments (2)
netmanager-app/components/Analytics/AnalyticsDropdown.tsx (2)
8-13: LGTM! Well-structured interface definition.The props interface is well-defined with clear types and good use of optional properties.
1-81: Overall, this is a well-implemented component.The component is well-structured, type-safe, and follows React best practices. It effectively handles both cohort and grid data types while maintaining a clean and accessible UI. The suggested improvements are mostly optimizations rather than critical issues.
| const handleAirqloudChange = (value: string) => { | ||
| const selectedAirqloud = airqloudsData.find((a) => a._id === value); | ||
| if (selectedAirqloud) { | ||
| const storageKey = isCohort ? "activeCohort" : "activeGrid"; | ||
| localStorage.setItem(storageKey, JSON.stringify(selectedAirqloud)); | ||
| onSelect(value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for localStorage operations.
The handleAirqloudChange function should handle potential localStorage failures, which can occur in various scenarios (e.g., storage quota exceeded, private browsing mode).
const handleAirqloudChange = (value: string) => {
const selectedAirqloud = airqloudsData.find((a) => a._id === value);
if (selectedAirqloud) {
const storageKey = isCohort ? "activeCohort" : "activeGrid";
- localStorage.setItem(storageKey, JSON.stringify(selectedAirqloud));
- onSelect(value);
+ try {
+ localStorage.setItem(storageKey, JSON.stringify(selectedAirqloud));
+ onSelect(value);
+ } catch (error) {
+ console.error('Failed to save selection to localStorage:', error);
+ onSelect(value); // Still update the UI even if storage fails
+ }
}
};📝 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 handleAirqloudChange = (value: string) => { | |
| const selectedAirqloud = airqloudsData.find((a) => a._id === value); | |
| if (selectedAirqloud) { | |
| const storageKey = isCohort ? "activeCohort" : "activeGrid"; | |
| localStorage.setItem(storageKey, JSON.stringify(selectedAirqloud)); | |
| onSelect(value); | |
| } | |
| }; | |
| const handleAirqloudChange = (value: string) => { | |
| const selectedAirqloud = airqloudsData.find((a) => a._id === value); | |
| if (selectedAirqloud) { | |
| const storageKey = isCohort ? "activeCohort" : "activeGrid"; | |
| try { | |
| localStorage.setItem(storageKey, JSON.stringify(selectedAirqloud)); | |
| onSelect(value); | |
| } catch (error) { | |
| console.error('Failed to save selection to localStorage:', error); | |
| onSelect(value); // Still update the UI even if storage fails | |
| } | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
netmanager-app/components/Analytics/GridDashboard.tsx (2)
23-28: Consider strengthening the PM2.5 type definition.The optional
pm2_5property could lead to runtime issues if not properly handled. Consider creating a dedicated type for the PM2.5 value structure.+interface PM25Reading { + value: number; +} + interface RecentEventFeature { properties: { site_id: string; - pm2_5?: { value: number }; + pm2_5?: PM25Reading; }; }
39-53: Reduce code duplication in category initialization.The PM2.5 categories are duplicated in both the state initialization and the
initialCountobject. Consider extracting this to a constant.+const INITIAL_PM25_CATEGORIES = { + Good: [], + Moderate: [], + UHFSG: [], + Unhealthy: [], + VeryUnhealthy: [], + Hazardous: [], +} as const; -const [pm2_5SiteCount, setPm2_5SiteCount] = useState<{ - Good: Site[]; - Moderate: Site[]; - UHFSG: Site[]; - Unhealthy: Site[]; - VeryUnhealthy: Site[]; - Hazardous: Site[]; -}>({ - Good: [], - Moderate: [], - UHFSG: [], - Unhealthy: [], - VeryUnhealthy: [], - Hazardous: [], -}); +const [pm2_5SiteCount, setPm2_5SiteCount] = useState<{ + [K in keyof typeof INITIAL_PM25_CATEGORIES]: Site[]; +}>({ ...INITIAL_PM25_CATEGORIES }); // Later in the code -const initialCount = { - Good: [], - Moderate: [], - UHFSG: [], - Unhealthy: [], - VeryUnhealthy: [], - Hazardous: [], -}; +const initialCount = { ...INITIAL_PM25_CATEGORIES };Also applies to: 69-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netmanager-app/app/types/grids.ts(1 hunks)netmanager-app/components/Analytics/AnalyticsDropdown.tsx(1 hunks)netmanager-app/components/Analytics/GridDashboard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netmanager-app/components/Analytics/AnalyticsDropdown.tsx
- netmanager-app/app/types/grids.ts
🔇 Additional comments (3)
netmanager-app/components/Analytics/GridDashboard.tsx (3)
60-60: Replace 'any' type with proper interface.The
categoriesparameter incategorizeSitefunction uses theanytype.
183-188: Verify the empty analyticsDevices array prop.The
ExceedancesChartcomponent receives an empty array foranalyticsDevices. Verify if this is intentional or if device data should be passed.Run this script to check if the component expects device data:
✅ Verification successful
Empty analyticsDevices array is correct for grid mode
The component uses
analyticsSitesexclusively whenisGridsis true, making the emptyanalyticsDevicesarray appropriate in this context. The device data is only required when operating in cohorts mode (isCohorts=true).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ExceedancesChart usage and implementation ast-grep --pattern 'ExceedancesChart'Length of output: 1340
Script:
#!/bin/bash # Check ExceedeanceLine.tsx implementation and other usages echo "=== ExceedeanceLine.tsx content ===" cat netmanager-app/components/Analytics/ExceedeanceLine.tsx.tsx echo -e "\n=== Other GridDashboard usages ===" ast-grep --pattern 'ExceedancesChart[^}]*isGrids'Length of output: 10591
88-88: Reconsider using 0 as fallback for missing PM2.5 values.Using 0 as a fallback for missing PM2.5 values could misrepresent the air quality data. Consider either:
- Excluding sites with missing PM2.5 values
- Using a designated "No Data" category
-const pm2_5 = feature.properties.pm2_5?.value || 0; +const pm2_5 = feature.properties.pm2_5?.value; +if (pm2_5 === undefined) return;
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
netmanager-app/components/Analytics/ExceedanceLine.tsx (2)
71-99: Consider implementing request cancellation for data fetching.The
useEffecthook fetching data should handle cleanup to prevent memory leaks and race conditions when component unmounts or dependencies change.useEffect(() => { + const controller = new AbortController(); const fetchData = async () => { setLoading(true); const filter = { pollutant: pollutant.value, standard: standard.value, startDate: new Date(Date.now() - 28 * 24 * 60 * 60 * 1000).toISOString(), endDate: new Date().toISOString(), ...(isGrids ? { sites: averageSites } : { devices: averageDevices }), }; try { const response = await createAxiosInstance().post( isCohorts ? DEVICE_EXCEEDANCES_URI : EXCEEDANCES_DATA_URI, filter, + { signal: controller.signal } ); const data = response.data.data; processChartData(data); } catch (error) { + if (!error.name === 'AbortError') { console.error('Error fetching data:', error); setDataset([]); + } } setLoading(false); }; if ((isGrids && averageSites.length) || (isCohorts && averageDevices.length)) { fetchData(); } + return () => controller.abort(); }, [averageSites, averageDevices, isGrids, isCohorts, pollutant, standard]);
169-173: Commented code should be removed.Remove commented code that's not being used. If this feature is planned for future implementation, consider tracking it in your issue tracker instead.
netmanager-app/app/(authenticated)/data-export/page.tsx (1)
13-18: Enhance accessibility with ARIA labels.The description text should be associated with the form using ARIA attributes for better screen reader support.
- <p className="mb-4 text-center text-gray-600"> + <p className="mb-4 text-center text-gray-600" id="export-description" aria-describedby="export-form"> Customize the data you want to download. We recommend downloading data for shorter time periods like a week or a month to avoid timeouts. </p> + <div id="export-form" aria-describedby="export-description"> <Card>netmanager-app/components/Analytics/PollutantCategory.tsx (1)
8-11: Enhance type safety for Site interface.The
Siteinterface could be more specific about thepm2_5property type and potential null values.interface Site { label: string; - pm2_5: number; + pm2_5: number | null; }netmanager-app/components/Analytics/index.tsx (1)
41-53: Consider using a custom hook for local storage operations.The local storage logic could be extracted into a reusable custom hook for better maintainability and reusability.
// Suggested custom hook: function useLocalStorageData<T>(key: string, data: T[], defaultIndex = 0) { const [value, setValue] = useState<T | null>(null); useEffect(() => { const storedData = localStorage.getItem(key); if (storedData) { setValue(JSON.parse(storedData)); } else if (data.length > defaultIndex) { setValue(data[defaultIndex]); } }, [key, data, defaultIndex]); return [value, setValue] as const; }netmanager-app/components/Analytics/GridDashboard.tsx (3)
63-70: Optimize PM2.5 range check condition.The condition contains a redundant check. Since we're already checking
pm2_5 > min, thepm2_5 >= 0check is unnecessary.- if (pm2_5 >= 0 && pm2_5 > min && pm2_5 <= max) { + if (pm2_5 > min && pm2_5 <= max) {
159-174: Enhance accessibility of SVG icon.Consider adding dimensions to the aria-label to provide more context for screen readers.
- aria-label="Toggle chart type" + aria-label="Toggle chart type (15x15)"
188-193: Optimize component type identification props.Instead of using two boolean props (
isCohortsandisGrids), consider using a single discriminating prop with an enum or string literal type.- <ExceedancesChart - isCohorts={false} - isGrids={true} + <ExceedancesChart + type="grids" analyticsDevices={[]} analyticsSites={activeGrid?.sites || []} />netmanager-app/components/export-data/ExportForm.tsx (3)
170-202: Centralize error messages and simplify error handling.Consider extracting error messages to constants and simplifying the error handling logic.
+const ERROR_MESSAGES = { + SERVER_ERROR: "An error occurred while exporting your data. Please try again later.", + NO_RESPONSE: "No response received from server", + NO_DATA: "No data found for the selected parameters", +} as const; catch (error: any) { console.error("Error exporting data", error); - let errorMessage + let errorMessage = ERROR_MESSAGES.SERVER_ERROR; if (error.response) { - if (error.response.status >= 500) { - errorMessage = "An error occurred while exporting your data. Please try again later."; - } else { + if (error.response.status < 500) { if (error.response.data.status === 'success') { toast({ title: "Error exporting data", - description: 'No data found for the selected parameters', + description: ERROR_MESSAGES.NO_DATA, variant: "default", }) return; } errorMessage = typeof error.response.data.message === 'string' ? error.response.data : 'An error occurred while downloading data'; } } else if (error.request) { - errorMessage = 'No response received from server'; + errorMessage = ERROR_MESSAGES.NO_RESPONSE; - } else { - errorMessage = 'An error occurred while exporting your data. Please try again later.'; }
212-233: Extract magic numbers and simplify time difference calculation.Consider extracting the magic number and using a more readable date difference calculation.
+const MAX_DATE_RANGE_DAYS = 28; +const MS_PER_DAY = 1000 * 60 * 60 * 24; - const Difference_In_Time = new Date(data.endDateTime).getTime() - new Date(data.startDateTime).getTime(); - const Difference_In_Days = Difference_In_Time / (1000 * 3600 * 24); + const daysDifference = Math.floor( + (new Date(data.endDateTime).getTime() - new Date(data.startDateTime).getTime()) / MS_PER_DAY + ); - if (Difference_In_Days > 28) { + if (daysDifference > MAX_DATE_RANGE_DAYS) {
363-365: Add confirmation dialog for form reset.Consider adding a confirmation dialog before resetting the form to prevent accidental data loss.
- <Button variant="outline" onClick={() => reset()}> + <Button + variant="outline" + onClick={() => { + if (window.confirm('Are you sure you want to reset the form? All entered data will be lost.')) { + reset(); + } + }} + >
📜 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 (9)
netmanager-app/app/(authenticated)/data-export/page.tsx(1 hunks)netmanager-app/app/types/grids.ts(1 hunks)netmanager-app/components/Analytics/CohortsDashboard.tsx(1 hunks)netmanager-app/components/Analytics/ExceedanceLine.tsx(1 hunks)netmanager-app/components/Analytics/GridDashboard.tsx(1 hunks)netmanager-app/components/Analytics/PollutantCategory.tsx(1 hunks)netmanager-app/components/Analytics/index.tsx(1 hunks)netmanager-app/components/export-data/ExportForm.tsx(1 hunks)netmanager-app/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- netmanager-app/components/Analytics/CohortsDashboard.tsx
- netmanager-app/package.json
- netmanager-app/app/types/grids.ts
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager-app/components/Analytics/PollutantCategory.tsx
[error] 67-67: 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 (3)
netmanager-app/components/Analytics/PollutantCategory.tsx (1)
80-101: Color mapping implementation matches previous feedback.The color mapping implementation follows the suggested pattern from previous reviews, using a centralized color system.
netmanager-app/components/Analytics/GridDashboard.tsx (1)
22-38: Well-structured type definitions!The interfaces are properly typed with clear property definitions and appropriate use of optional chaining.
netmanager-app/components/export-data/ExportForm.tsx (1)
127-135: Improve security in file download implementation.The current implementation needs security enhancements to prevent XSS attacks.
| interface ExceedancesChartProps { | ||
| analyticsSites: any[]; | ||
| analyticsDevices: Device[]; | ||
| isGrids: boolean; | ||
| isCohorts: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Props interface needs type refinement.
The analyticsSites prop is typed as any[] which loses type safety. Consider creating a proper interface for the site structure.
interface ExceedancesChartProps {
- analyticsSites: any[];
+ analyticsSites: {
+ _id: string;
+ name?: string;
+ description?: string;
+ generated_name?: string;
+ }[];
analyticsDevices: Device[];
isGrids: boolean;
isCohorts: boolean;
}📝 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.
| interface ExceedancesChartProps { | |
| analyticsSites: any[]; | |
| analyticsDevices: Device[]; | |
| isGrids: boolean; | |
| isCohorts: boolean; | |
| } | |
| interface ExceedancesChartProps { | |
| analyticsSites: { | |
| _id: string; | |
| name?: string; | |
| description?: string; | |
| generated_name?: string; | |
| }[]; | |
| analyticsDevices: Device[]; | |
| isGrids: boolean; | |
| isCohorts: boolean; | |
| } |
| sortedData.map((data, index) => ( | ||
| <li key={index} className="p-2 hover:bg-gray-100"> | ||
| {data.label} - <span className={`font-semibold text-${getPm25LevelColor(pm25level)}`}> | ||
| {data.pm2_5 && !isNaN(data.pm2_5) ? data.pm2_5.toFixed(2) : 'N/A'} |
There was a problem hiding this comment.
Replace unsafe isNaN with Number.isNaN.
The global isNaN function performs type coercion which can lead to unexpected results.
- {data.pm2_5 && !isNaN(data.pm2_5) ? data.pm2_5.toFixed(2) : 'N/A'}
+ {data.pm2_5 && !Number.isNaN(data.pm2_5) ? data.pm2_5.toFixed(2) : 'N/A'}📝 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.
| {data.pm2_5 && !isNaN(data.pm2_5) ? data.pm2_5.toFixed(2) : 'N/A'} | |
| {data.pm2_5 && !Number.isNaN(data.pm2_5) ? data.pm2_5.toFixed(2) : 'N/A'} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: 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)
| const handleDownloadData = async () => { | ||
| setDownloadingData(true) | ||
| try { | ||
| await dataExport({ | ||
| sites: !isCohort ? activeGrid?.sites.map(site => site._id) : [], | ||
| devices: isCohort ? activeCohort?.devices.map(device => device._id) : [], | ||
| startDateTime: new Date(Date.now() - 5 * 24 * 60 * 60 * 1000).toISOString(), | ||
| endDateTime: new Date().toISOString(), | ||
| frequency: 'hourly', | ||
| pollutants: ['pm2_5', 'pm10'], | ||
| downloadType: isCohort ? 'csv' : 'json', | ||
| outputFormat: 'airqo-standard' | ||
| }) | ||
| toast({ | ||
| title: "Success", | ||
| description: "Air quality data download successful", | ||
| }) | ||
| } catch (error) { | ||
| toast({ | ||
| title: "Error", | ||
| description: "Error downloading data", | ||
| variant: "destructive", | ||
| }) | ||
| } finally { | ||
| setDownloadingData(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in data download.
The error handling could be more specific and provide better user feedback.
const handleDownloadData = async () => {
setDownloadingData(true)
try {
await dataExport({
sites: !isCohort ? activeGrid?.sites.map(site => site._id) : [],
devices: isCohort ? activeCohort?.devices.map(device => device._id) : [],
startDateTime: new Date(Date.now() - 5 * 24 * 60 * 60 * 1000).toISOString(),
endDateTime: new Date().toISOString(),
frequency: 'hourly',
pollutants: ['pm2_5', 'pm10'],
downloadType: isCohort ? 'csv' : 'json',
outputFormat: 'airqo-standard'
})
toast({
title: "Success",
description: "Air quality data download successful",
})
} catch (error) {
+ const errorMessage = error instanceof Error
+ ? error.message
+ : "An unexpected error occurred while downloading data";
toast({
title: "Error",
- description: "Error downloading data",
+ description: errorMessage,
variant: "destructive",
})
+ console.error('Download error:', error);
} finally {
setDownloadingData(false)
}
}📝 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 handleDownloadData = async () => { | |
| setDownloadingData(true) | |
| try { | |
| await dataExport({ | |
| sites: !isCohort ? activeGrid?.sites.map(site => site._id) : [], | |
| devices: isCohort ? activeCohort?.devices.map(device => device._id) : [], | |
| startDateTime: new Date(Date.now() - 5 * 24 * 60 * 60 * 1000).toISOString(), | |
| endDateTime: new Date().toISOString(), | |
| frequency: 'hourly', | |
| pollutants: ['pm2_5', 'pm10'], | |
| downloadType: isCohort ? 'csv' : 'json', | |
| outputFormat: 'airqo-standard' | |
| }) | |
| toast({ | |
| title: "Success", | |
| description: "Air quality data download successful", | |
| }) | |
| } catch (error) { | |
| toast({ | |
| title: "Error", | |
| description: "Error downloading data", | |
| variant: "destructive", | |
| }) | |
| } finally { | |
| setDownloadingData(false) | |
| } | |
| } | |
| const handleDownloadData = async () => { | |
| setDownloadingData(true) | |
| try { | |
| await dataExport({ | |
| sites: !isCohort ? activeGrid?.sites.map(site => site._id) : [], | |
| devices: isCohort ? activeCohort?.devices.map(device => device._id) : [], | |
| startDateTime: new Date(Date.now() - 5 * 24 * 60 * 60 * 1000).toISOString(), | |
| endDateTime: new Date().toISOString(), | |
| frequency: 'hourly', | |
| pollutants: ['pm2_5', 'pm10'], | |
| downloadType: isCohort ? 'csv' : 'json', | |
| outputFormat: 'airqo-standard' | |
| }) | |
| toast({ | |
| title: "Success", | |
| description: "Air quality data download successful", | |
| }) | |
| } catch (error) { | |
| const errorMessage = error instanceof Error | |
| ? error.message | |
| : "An unexpected error occurred while downloading data"; | |
| toast({ | |
| title: "Error", | |
| description: errorMessage, | |
| variant: "destructive", | |
| }) | |
| console.error('Download error:', error); | |
| } finally { | |
| setDownloadingData(false) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
netmanager-app/components/export-data/ExportForm.tsx (4)
138-206: Enhance type safety and error handling in downloadData function.The error handling is comprehensive, but there are opportunities for improvement:
- File type handling could be more type-safe
- Error messages could be more specific
Consider using a type-safe approach for file types:
+type FileType = 'csv' | 'json'; + +interface FileTypeConfig { + mimeType: string; + process: (data: any) => string; +} + +const fileTypeConfigs: Record<FileType, FileTypeConfig> = { + csv: { + mimeType: 'text/csv;charset=utf-8;', + process: (data) => data as string, + }, + json: { + mimeType: 'application/json', + process: (data) => JSON.stringify(data.data), + }, +};
223-224: Extract magic numbers into named constants.The 28-day limit and time calculations should be more maintainable and self-documenting.
+const MAX_DATE_RANGE_DAYS = 28; +const MS_PER_DAY = 1000 * 60 * 60 * 24; + -const Difference_In_Time = new Date(data.endDateTime).getTime() - new Date(data.startDateTime).getTime(); -const Difference_In_Days = Difference_In_Time / (1000 * 3600 * 24); +const differenceInDays = (new Date(data.endDateTime).getTime() - new Date(data.startDateTime).getTime()) / MS_PER_DAY; -if (Difference_In_Days > 28) { +if (differenceInDays > MAX_DATE_RANGE_DAYS) {Also applies to: 226-226
311-322: Improve type safety in renderFieldBasedOnTab function.The switch statement could benefit from exhaustive type checking.
+const exportTypes = ['sites', 'devices', 'airqlouds'] as const; +type ExportType = typeof exportTypes[number]; const renderFieldBasedOnTab = () => { switch (exportType) { case "sites": return renderMultiSelect("sites", siteOptions, "Select sites", { required: "At least one site must be selected" }); case "devices": return renderMultiSelect("devices", deviceOptions, "Select devices", { required: "At least one device must be selected" }); case "airqlouds": return renderMultiSelect("cities", cityOptions, "Select Grids", { required: "At least one Grids must be selected" }); default: - return null; + const _exhaustiveCheck: never = exportType; + return _exhaustiveCheck; } };
324-375: Enhance accessibility and user experience.The form could benefit from improved accessibility and user interaction:
- Add aria labels for screen readers
- Add confirmation for reset action
- Provide more detailed loading states
<form + aria-label="Export Data Form" onSubmit={handleSubmit(onSubmit)} className="space-y-4 p-4" > // ... other form elements ... <Button type="submit" disabled={loading} + aria-busy={loading} > - {loading ? "Exporting..." : "Export Data"} + {loading ? "Preparing Export..." : "Export Data"} </Button> <Button variant="outline" - onClick={() => reset()} + onClick={() => { + if (window.confirm('Are you sure you want to reset the form?')) { + reset(); + } + }} + aria-label="Reset form" > Reset </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netmanager-app/components/export-data/ExportForm.tsx(1 hunks)
🔇 Additional comments (1)
netmanager-app/components/export-data/ExportForm.tsx (1)
52-71: Consider timezone handling in date utility functions.The date utility functions use UTC hours which might cause issues with local timezone handling. Consider using local time or making the timezone handling configurable.
Would you like me to generate a more robust implementation that handles timezones appropriately?
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
Based on the comprehensive changes, here are the high-level release notes:
New Features
Improvements
Bug Fixes
Performance