Analytics: Download toast added #2200
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve several modifications across various components in the platform. Key updates include renaming and restructuring the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
New next-platform changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
platform/public/icons/Analytics/checkCircleIcon.js (2)
3-3: Consider adding prop-types for type safety.While the destructuring of props is clean, adding PropTypes would improve type safety and documentation.
import React from 'react'; +import PropTypes from 'prop-types'; const CheckCircleIcon = ({ width, height }) => { // ... component implementation ... }; +CheckCircleIcon.propTypes = { + width: PropTypes.number, + height: PropTypes.number +};Also applies to: 6-7
Line range hint
6-20: Consider enhancing SVG accessibility.While the SVG implementation is clean, it could benefit from accessibility improvements.
<svg width={width || 20} height={height || 20} viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg" + role="img" + aria-label="Check circle icon" >platform/src/common/components/Toast/CustomToast.jsx (1)
1-20: Consider adding PropTypes or TypeScript for better type safety.Since this component might be used across different parts of the application (as mentioned in the AI summary), adding proper type checking would improve maintainability and prevent potential runtime errors.
Example implementation with PropTypes:
import PropTypes from 'prop-types'; // ... component code ... CustomToast.propTypes = { message: PropTypes.string, duration: PropTypes.number, className: PropTypes.string, };platform/src/core/utils/useResizeObserver.js (1)
1-32: Consider these architectural improvements for better maintainability.
- Add TypeScript types for better type safety and IDE support
- Consider memoizing the size object to prevent unnecessary re-renders
- Add debouncing for high-frequency resize events
Here's how you could implement these improvements:
import { useState, useEffect, useMemo } from 'react'; import debounce from 'lodash/debounce'; interface Size { width: number; height: number; } interface ResizeObserverProps { ref: React.RefObject<HTMLElement>; initialSize?: Size; debounceMs?: number; } const useResizeObserver = ({ ref, initialSize = { width: 0, height: 0 }, debounceMs = 100 }: ResizeObserverProps): Size => { const [size, setSize] = useState<Size>(initialSize); const debouncedSetSize = useMemo( () => debounce(setSize, debounceMs), [debounceMs] ); useEffect(() => { return () => { debouncedSetSize.cancel(); }; }, [debouncedSetSize]); // ... rest of the implementation return useMemo(() => size, [size.width, size.height]); };platform/src/common/components/Charts/utils/index.jsx (2)
38-38: Add JSDoc documentation for thestepparameter.The new
stepparameter lacks documentation. Consider adding a description to help other developers understand its purpose and expected values./** * CustomizedAxisTick Component * Formats the X-axis ticks based on the frequency passed as a prop. + * @param {number} step - Controls the interval between displayed tick labels. When > 1, shows every nth tick. */
77-82: Consider extracting magic numbers as named constants.The rotation logic is well-structured, but the magic numbers could be extracted as named constants for better maintainability and reusability.
+// Constants for tick label rotation +const ROTATION_ANGLE = -45; +const ROTATED_DY = '10'; +const DEFAULT_DY = '16'; + // Determine if rotation is needed const rotate = step > 1; -const rotationAngle = rotate ? -45 : 0; +const rotationAngle = rotate ? ROTATION_ANGLE : 0; const textAnchor = rotate ? 'end' : 'middle'; -const dy = rotate ? '10' : '16'; +const dy = rotate ? ROTATED_DY : DEFAULT_DY;platform/src/common/components/Charts/ChartContainer.jsx (3)
Line range hint
8-8: Clean up commented-out codeThe commented-out
PrintReportModalimport and its corresponding JSX implementation should be removed if they're no longer needed. Keeping commented code can lead to confusion and maintenance overhead.Also applies to: 272-283
Line range hint
201-210: Enhance error handling UXConsider these improvements to the error overlay:
- Add a loading state to the "Try again" button during retry
- Provide more specific error messages to help users understand what went wrong
- <p className="text-red-500 font-semibold"> - Something went wrong. Please try again. - </p> + <p className="text-red-500 font-semibold"> + {error?.message || 'Unable to load chart data. Please try again.'} + </p> <button onClick={refetch} - className="ml-4 text-red-500 font-semibold underline" + className="ml-4 text-red-500 font-semibold underline disabled:opacity-50" + disabled={chartLoading} > - Try again + {chartLoading ? 'Retrying...' : 'Try again'} </button>
Line range hint
246-258: Consider empty state handling and prop consistencyTwo suggestions for improvement:
- Add an empty state UI when
allSiteDatais empty instead of showing nothing- Use the
widthprop passed to the component instead of hardcoding "100%"- {!chartLoading && !error && allSiteData?.length > 0 && ( + {!chartLoading && !error && ( + allSiteData?.length > 0 ? ( <MoreInsightsChart data={allSiteData} selectedSites={chartSites} chartType={chartType} frequency={timeFrame} - width="100%" + width={width} height={height} id={id} pollutantType={pollutionType} isLoading={chartLoading} /> + ) : ( + <div className="flex items-center justify-center h-full"> + <p className="text-gray-500">No data available for the selected criteria</p> + </div> + ) )}platform/src/common/components/Charts/MoreInsightsChart.jsx (3)
162-171: Consider adding boundary checks to the step calculation.The step calculation logic is sound, but could benefit from additional safeguards.
Consider this enhanced version:
const calculateStep = useCallback(() => { const minLabelWidth = 40; - if (containerWidth === 0) return 1; + if (!containerWidth || containerWidth <= 0) return 1; + if (!chartData || chartData.length === 0) return 1; const maxLabels = Math.floor(containerWidth / minLabelWidth); const step = Math.ceil(chartData.length / maxLabels); - return step; + return Math.max(1, step); // Ensure step is never less than 1 }, [containerWidth, chartData.length]);
Line range hint
182-196: Verify loading state logic.The condition
chartData.length === 0 && isLoadingmight not cover all edge cases. It shows "No Data Available" when loading, which seems counterintuitive.Consider updating the condition:
- if (chartData.length === 0 && isLoading) { + if (chartData.length === 0 && !isLoading) {
Line range hint
197-223: Consider extracting magic numbers into named constants.The margin values and padding values could be more maintainable as named constants.
Consider this approach:
const CHART_MARGINS = { top: 38, right: 10, left: -15, bottom: 20 }; const AXIS_PADDING = { left: 30, right: 30 };Then use them in your component:
- margin={{ top: 38, right: 10, left: -15, bottom: 20 }} + margin={CHART_MARGINS}platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (2)
22-22: Consider using an absolute import path.The relative import path with multiple parent traversals (
../../../) could make the code brittle to refactoring. Consider using an absolute import path (e.g.,@/common/components/Toast/CustomToast) for better maintainability.
Line range hint
92-231: Consider enhancing error handling with more specific messages.The handleSubmit function has good error handling, but could be improved:
- Add specific error messages for different file type failures
- Consider implementing retry logic for failed downloads
- Add error tracking/logging for better debugging
Example implementation for specific error messages:
} catch (error) { - console.error('Error downloading data:', error); - setFormError('An error occurred while downloading. Please try again.'); + console.error('Error downloading data:', error); + let errorMessage = 'An error occurred while downloading. Please try again.'; + + if (error.message === 'Invalid CSV data format') { + errorMessage = 'The downloaded CSV file format is invalid. Please try again.'; + } else if (error.message === 'Invalid JSON data format') { + errorMessage = 'The downloaded JSON data is malformed. Please try again.'; + } else if (error.message === 'Invalid PDF data format') { + errorMessage = 'Unable to generate PDF with the provided data. Please try again.'; + } + + setFormError(errorMessage); + CustomToast({ + message: errorMessage, + type: 'error' + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- platform/public/icons/Analytics/checkCircleIcon.js (2 hunks)
- platform/src/common/components/Charts/ChartContainer.jsx (6 hunks)
- platform/src/common/components/Charts/MoreInsightsChart.jsx (8 hunks)
- platform/src/common/components/Charts/utils/index.jsx (2 hunks)
- platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (2 hunks)
- platform/src/common/components/Toast/CustomToast.jsx (1 hunks)
- platform/src/core/utils/useResizeObserver.js (1 hunks)
🔇 Additional comments (12)
platform/public/icons/Analytics/checkCircleIcon.js (1)
3-3: LGTM! Component naming follows React conventions.The renaming to
CheckCircleIconproperly follows React component naming conventions using PascalCase.Also applies to: 23-23
platform/src/common/components/Toast/CustomToast.jsx (3)
1-2: LGTM! Clean and necessary imports.The imports are correctly structured, importing only what's needed.
20-20: LGTM! Proper export statement.The default export is appropriate for this single component.
1-20: Verify the impact on existing toast implementations.According to the AI summary, this component replaces existing toast implementations in multiple files. Let's verify the changes are consistent across the codebase.
platform/src/core/utils/useResizeObserver.js (3)
1-6: LGTM: Clean imports and well-documented hook declaration.The imports are minimal and the hook is properly documented with JSDoc comments.
29-32: LGTM: Clean export of the hook.The hook's return value and export are properly implemented.
9-27: 🛠️ Refactor suggestionVerify cleanup behavior and add error handling.
The effect cleanup looks good, but there are a few improvements we could make:
- Add error handling for ResizeObserver initialization
- Handle the case where ref.current becomes null during observation
useEffect(() => { if (!ref.current) return; const observeTarget = ref.current; - const resizeObserver = new ResizeObserver((entries) => { - for (let entry of entries) { - setSize({ - width: entry.contentRect.width, - height: entry.contentRect.height, - }); - } - }); + let resizeObserver; + try { + resizeObserver = new ResizeObserver((entries) => { + if (!entries.length) return; + const entry = entries[0]; + setSize({ + width: entry.contentRect.width, + height: entry.contentRect.height, + }); + }); + resizeObserver.observe(observeTarget); + } catch (error) { + console.error('ResizeObserver error:', error); + return; + } - resizeObserver.observe(observeTarget); return () => { - resizeObserver.unobserve(observeTarget); + if (resizeObserver) { + resizeObserver.disconnect(); + } }; }, [ref]);Let's verify the ResizeObserver support in the codebase:
platform/src/common/components/Charts/utils/index.jsx (2)
66-69: Clean and efficient tick display logic!The simplified logic using the step parameter is more maintainable and provides better control over tick density. Good job on handling the edge case when step ≤ 1.
88-92: Well-implemented dynamic text attributes!The conditional attributes for text positioning and rotation are cleanly implemented. Good use of undefined to conditionally apply the transform attribute.
platform/src/common/components/Charts/ChartContainer.jsx (1)
116-116: Enhance CustomToast implementationThe CustomToast call might need parameters to provide meaningful feedback about the download completion. Consider passing the format type and success status.
platform/src/common/components/Charts/MoreInsightsChart.jsx (2)
Line range hint
1-54: Nice addition of the resize observer hook! 👍The integration of
useResizeObserverwith a container ref is a solid improvement for responsive behavior. This approach is more reliable than depending on window events.
Line range hint
314-328: Verify dependency array completeness.The renderChart useMemo dependency array includes all necessary dependencies, which is great! However, let's verify that all dynamic values used within the chart rendering are properly tracked.
✅ Verification successful
All dependencies are correctly included in the dependency array
Looking at the complete implementation of the
renderChartuseMemo hook, I can verify that all variables used within the function are properly included in the dependency array. The array contains:
- Chart-related dependencies:
ChartComponent,DataComponent,chartData,chartType,width,height- Interaction handlers:
handleMouseLeave,activeIndex- Data formatting:
step,getColor,pollutantType,WHO_STANDARD_VALUE,dataKeys- Display helpers:
renderCustomizedLegend,frequency,siteIdToName,isLoadingEach of these dependencies corresponds to values used within the chart rendering logic, with no missing dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any variables used in the renderChart function that might be missing from the dependency array ast-grep --pattern 'const renderChart = useMemo(() => { $$$ return ( $$$ ); }, [$$$]);'Length of output: 13883
| const CustomToast = () => { | ||
| toast('Download complete', { | ||
| className: | ||
| 'bg-blue-600 text-white p-3 rounded-xl max-w-[200px] w-full flex items-center justify-center space-x-2', | ||
| duration: 5000, | ||
| position: 'bottom-center', | ||
| style: { | ||
| left: '50%', | ||
| transform: 'translateX(10%)', | ||
| bottom: '10px', | ||
| position: 'fixed', | ||
| }, | ||
| icon: <CheckCircleIcon width={20} height={20} className="text-white" />, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Consider making the toast component more flexible and fix the positioning.
A few suggestions to improve this component:
- The transform value
translateX(10%)will shift the toast to the right. For proper centering, usetranslateX(-50%). - The
position: 'fixed'style is redundant as it's already handled by theposition: 'bottom-center'prop. - Consider making the component more reusable by accepting props for customization.
Here's a suggested improvement:
-const CustomToast = () => {
+const CustomToast = ({
+ message = 'Download complete',
+ duration = 5000,
+ className = 'bg-blue-600 text-white',
+}) => {
toast('Download complete', {
className:
- 'bg-blue-600 text-white p-3 rounded-xl max-w-[200px] w-full flex items-center justify-center space-x-2',
+ `${className} p-3 rounded-xl max-w-[200px] w-full flex items-center justify-center space-x-2`,
- duration: 5000,
+ duration,
position: 'bottom-center',
style: {
left: '50%',
- transform: 'translateX(10%)',
+ transform: 'translateX(-50%)',
bottom: '10px',
- position: 'fixed',
},
icon: <CheckCircleIcon width={20} height={20} className="text-white" />,
});
};📝 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 CustomToast = () => { | |
| toast('Download complete', { | |
| className: | |
| 'bg-blue-600 text-white p-3 rounded-xl max-w-[200px] w-full flex items-center justify-center space-x-2', | |
| duration: 5000, | |
| position: 'bottom-center', | |
| style: { | |
| left: '50%', | |
| transform: 'translateX(10%)', | |
| bottom: '10px', | |
| position: 'fixed', | |
| }, | |
| icon: <CheckCircleIcon width={20} height={20} className="text-white" />, | |
| }); | |
| }; | |
| const CustomToast = ({ | |
| message = 'Download complete', | |
| duration = 5000, | |
| className = 'bg-blue-600 text-white', | |
| }) => { | |
| toast('Download complete', { | |
| className: | |
| `${className} p-3 rounded-xl max-w-[200px] w-full flex items-center justify-center space-x-2`, | |
| duration, | |
| position: 'bottom-center', | |
| style: { | |
| left: '50%', | |
| transform: 'translateX(-50%)', | |
| bottom: '10px', | |
| }, | |
| icon: <CheckCircleIcon width={20} height={20} className="text-white" />, | |
| }); | |
| }; |
| * Custom hook to observe the size of a DOM element using ResizeObserver. | ||
| */ | ||
| const useResizeObserver = (ref) => { | ||
| const [size, setSize] = useState({ width: 0, height: 0 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding initial size prop for better SSR support.
The initial state of { width: 0, height: 0 } might cause layout shifts during server-side rendering.
-const useResizeObserver = (ref) => {
- const [size, setSize] = useState({ width: 0, height: 0 });
+const useResizeObserver = (ref, initialSize = { width: 0, height: 0 }) => {
+ const [size, setSize] = useState(initialSize);Committable suggestion was skipped due to low confidence.
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); // Remove after clicking | ||
| document.body.removeChild(link); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using URL.createObjectURL for downloads
Instead of manipulating the DOM directly, consider using URL.createObjectURL for a cleaner approach to handling downloads.
- document.body.appendChild(link);
- link.click();
- document.body.removeChild(link);
+ const url = URL.createObjectURL(new Blob([imgData]));
+ link.href = url;
+ link.click();
+ URL.revokeObjectURL(url);Committable suggestion was skipped due to low confidence.
| // Show success toast | ||
| CustomToast(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance toast notifications for better user feedback.
The current implementation could be improved in several ways:
- Pass a meaningful success message to CustomToast
- Add error toasts for failed downloads
- Consider adding a loading toast for long downloads
- CustomToast();
+ CustomToast({
+ message: `Successfully downloaded ${formData.title.name}.${formData.fileType.name.toLowerCase()}`,
+ type: 'success'
+ });And in the catch block:
console.error('Error downloading data:', error);
setFormError('An error occurred while downloading. Please try again.');
+ CustomToast({
+ message: 'Failed to download data. Please try again.',
+ type: 'error'
+ });Committable suggestion was skipped due to low confidence.
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
platform/src/common/components/Charts/MoreInsightsChart.jsx (1)
Line range hint
182-196: Loading state logic could be more robustThe condition
chartData.length === 0 && isLoadingmight not cover all edge cases. It could show "No Data Available" during loading.Consider updating the condition:
-if (chartData.length === 0 && isLoading) { +if (isLoading) { + return <SkeletonLoader width={width} height={height} />; +} +if (chartData.length === 0) { return ( <div className="w-full flex flex-col justify-center items-center h-[380px] text-gray-500"> <p className="text-lg font-medium mb-2">No Data Available</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- platform/src/common/components/Charts/MoreInsightsChart.jsx (9 hunks)
🔇 Additional comments (3)
platform/src/common/components/Charts/MoreInsightsChart.jsx (3)
Line range hint
314-328: Dependencies list in useMemo looks goodThe addition of
stepto the dependency array is correct. The ref usage for the container is properly implemented.
Line range hint
197-223: Chart configuration updates look good, but verify the margin changesThe margin adjustment (
-15on the left) seems unusual. This might cause axis labels to be cut off on smaller screens.Let's check if there are any similar margin patterns in other chart components:
#!/bin/bash # Look for other chart margin configurations rg -A 2 "margin[=\s]*{" --type jsxConsider using positive margins and adjusting the container padding instead:
-margin={{ top: 38, right: 10, left: -15, bottom: 20 }} +margin={{ top: 38, right: 10, left: 10, bottom: 20 }}
Line range hint
1-54: Nice addition of the resize observer pattern! 👍The integration of
useResizeObserveris a solid improvement for responsive behavior. However, there's a potential edge case to consider.Let's verify the resize observer implementation:
Consider adding a default width in the destructuring to handle the initial render:
-const { width: containerWidth } = useResizeObserver(containerRef); +const { width: containerWidth = 0 } = useResizeObserver(containerRef);
| * Calculate step based on container width and number of ticks. | ||
| * Assume each label requires a minimum width | ||
| */ | ||
| const calculateXAxisInterval = useCallback(() => { | ||
| const screenWidth = window.innerWidth; | ||
| if (screenWidth < 768) return Math.ceil(chartData.length / 4); | ||
| if (screenWidth < 1024) return Math.ceil(chartData.length / 6); | ||
| return Math.ceil(chartData.length / 8); | ||
| }, [chartData.length]); | ||
| const calculateStep = useCallback(() => { | ||
| const minLabelWidth = 40; | ||
| if (containerWidth === 0) return 1; | ||
| const maxLabels = Math.floor(containerWidth / minLabelWidth); | ||
| const step = Math.ceil(chartData.length / maxLabels); | ||
| return step; | ||
| }, [containerWidth, chartData.length]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Smart approach to dynamic step calculation! 🎯
The calculateStep function elegantly handles the label spacing based on container width. The magic number 40 for minLabelWidth could be made more maintainable.
Consider extracting the configuration:
+const CHART_CONFIG = {
+ MIN_LABEL_WIDTH: 40,
+ DEFAULT_STEP: 1
+};
const calculateStep = useCallback(() => {
- const minLabelWidth = 40;
+ const minLabelWidth = CHART_CONFIG.MIN_LABEL_WIDTH;
if (containerWidth === 0) return 1;
const maxLabels = Math.floor(containerWidth / minLabelWidth);
const step = Math.ceil(chartData.length / maxLabels);
return step;
}, [containerWidth, chartData.length]);Committable suggestion was skipped due to low confidence.
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
platform/src/core/utils/useFetchAnalyticsData.jsx (2)
Line range hint
1-15: Consider changing file extension to .jsSince this file contains a custom hook rather than a JSX component, it should use the .js extension instead of .jsx.
Enhance JSDoc documentation
While the current documentation provides a basic description, it would be more helpful to include:
- @param descriptions for each parameter
- @returns description of the returned object
- @example showing typical usage
Here's a suggested improvement:
/** * Custom hook to fetch analytics data based on provided parameters. + * @param {Object} options - The options object + * @param {Array<string>} options.selectedSiteIds - Array of site IDs to fetch data for + * @param {Object} options.dateRange - Object containing startDate and endDate + * @param {string} options.chartType - Type of chart ('line' | 'bar') + * @param {string} options.frequency - Data frequency ('daily' | 'hourly') + * @param {string} options.pollutant - Type of pollutant to analyze + * @param {string} options.organisationName - Name of the organization + * @returns {Object} { + * allSiteData: Array<Object> - The fetched analytics data + * chartLoading: boolean - Loading state indicator + * error: string|null - Error message if any + * refetch: Function - Function to manually trigger data fetch + * } + * @example + * const { allSiteData, chartLoading, error, refetch } = useFetchAnalyticsData({ + * selectedSiteIds: ['site1', 'site2'], + * dateRange: { startDate: new Date(), endDate: new Date() }, + * chartType: 'line' + * }); */
Line range hint
19-60: Enhance error handling and add cleanupWhile the error handling is generally good, there are a few improvements to consider:
- Add cleanup for AbortController:
const fetchAnalyticsData = useCallback(async () => { setChartLoading(true); setError(null); + const controller = new AbortController(); try { if (selectedSiteIds.length === 0) { // No sites selected, clear data and error, set loading to false setAllSiteData([]); setError(null); setChartLoading(false); return; } const requestBody = { sites: selectedSiteIds, startDate: parseAndValidateISODate(dateRange.startDate), endDate: parseAndValidateISODate(dateRange.endDate), chartType, frequency, pollutant, organisation_name: organisationName, }; - const controller = new AbortController(); const response = await getAnalyticsData({ body: requestBody, signal: controller.signal, }); if (response.status === 'success' && Array.isArray(response.data)) { setAllSiteData(response.data); } else { setAllSiteData([]); throw new Error(response.message || 'Failed to fetch analytics data.'); } } catch (err) { + // Don't set error state if request was aborted + if (err.name === 'AbortError') { + return; + } console.error('Error fetching analytics data:', err); setError(err.message || 'An unexpected error occurred.'); setAllSiteData([]); } finally { setChartLoading(false); } + + return () => controller.abort(); }, [/* dependencies */]);
- Add more specific error handling for different scenarios (network errors, validation errors, etc.)
- Add response data validation to ensure the expected structure is present
platform/src/common/components/Charts/ChartContainer.jsx (4)
165-175: Consider removing or documenting commented code sectionsThe file contains multiple blocks of commented-out code related to report sharing functionality. While preserving code for reference can be useful, it's generally better to:
- Remove it if it's no longer needed
- Move it to a separate branch if it's for future implementation
- Add a TODO comment explaining why it's retained
If this code is planned for future use, consider adding a TODO comment with a ticket reference.
Also applies to: 250-264
Line range hint
102-106: Enhance error handling in exportChartCurrently, export errors are only logged to the console. Consider notifying users when exports fail.
} catch (error) { console.error('Error exporting chart:', error); + CustomToast({ + message: 'Failed to export chart. Please try again.', + type: 'error' + }); }
Line range hint
234-246: Consider simplifying chart rendering logicThe nested conditions for rendering different states could be simplified using early returns or a more declarative approach.
Consider refactoring to:
const renderChart = () => { if (chartLoading) return <SkeletonLoader width={width} height={height} />; if (error) return <ErrorOverlay />; if (!data?.length) return null; return ( <MoreInsightsChart data={data} selectedSites={chartSites} chartType={chartType} frequency={timeFrame} width="100%" height={height} id={id} pollutantType={pollutionType} isLoading={chartLoading} /> ); };
277-280: Consider enhancing PropTypes definitionsA few suggestions for the prop types:
- The
errorprop might need to handle both string and Error object types- The
refetchprop would benefit from JSDoc documentation explaining its usage- error: PropTypes.string, + error: PropTypes.oneOfType([ + PropTypes.string, + PropTypes.instanceOf(Error) + ]), + /** + * Callback function to refresh chart data + * @returns {Promise<void>} + */ refetch: PropTypes.func.isRequired,platform/src/common/components/Charts/MoreInsightsChart.jsx (1)
182-186: Well-structured loading and empty states! 🎯Good separation of concerns between loading and empty states. Consider adding an error state handler for failed data fetches to complete the user feedback loop.
if (isLoading) { return <SkeletonLoader width={width} height={height} />; } +if (error) { + return ( + <div className="w-full flex flex-col justify-center items-center h-[380px] text-gray-500"> + <p className="text-lg font-medium mb-2">Error Loading Data</p> + <p className="text-sm">{error.message || 'Please try again later.'}</p> + </div> + ); +} if (chartData.length === 0) {platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (4)
Line range hint
100-106: Remove unusedonToggleprop fromLocationCard.In the
LocationCardcomponent, theonToggleprop is passed an empty function. If this prop isn't being used withinLocationCard, consider removing it to simplify the code.Apply this diff to remove the unused prop:
return selectedSites.map((site) => ( <LocationCard key={site._id} site={site} - onToggle={() => {}} isSelected={true} isLoading={false} /> ));
Line range hint
49-56: Clean up commented-out code related to modals and actions.There are several sections where code related to modals and actions, such as
handleOpenModal,Add Locationbutton, andDownload Databutton, is commented out. If these features are intended to be part of the component, consider uncommenting and integrating them. If they're no longer needed, removing them will help keep the codebase clean and maintainable.Also applies to: 218-222, 264-268
57-67: SimplifyinitialDateRangecomputation withoutuseMemo.Since
initialDateRangeis computed once during initialization and doesn't depend on props or state, usinguseMemomight be unnecessary. You could compute it directly withoutuseMemofor simplicity.Apply this diff to simplify:
- const initialDateRange = useMemo(() => { - const { startDateISO, endDateISO } = formatDateRangeToISO( - subDays(new Date(), 7), - new Date(), - ); - return { - startDate: startDateISO, - endDate: endDateISO, - label: 'Last 7 days', - }; - }, []); + const { startDateISO, endDateISO } = formatDateRangeToISO( + subDays(new Date(), 7), + new Date(), + ); + const initialDateRange = { + startDate: startDateISO, + endDate: endDateISO, + label: 'Last 7 days', + };
Line range hint
80-82: Ensure all dependencies are included inuseCallback.In your
handleParameterChangefunction, make sure all variables and functions used inside are included in the dependency array. Currently, onlyrefetchis included, which is good if that's the only dependency, but double-check if any other variables are used within the callback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- platform/src/common/components/Charts/ChartContainer.jsx (10 hunks)
- platform/src/common/components/Charts/MoreInsightsChart.jsx (8 hunks)
- platform/src/common/components/Charts/utils/index.jsx (2 hunks)
- platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (8 hunks)
- platform/src/common/components/Toast/CustomToast.jsx (1 hunks)
- platform/src/core/utils/useFetchAnalyticsData.jsx (2 hunks)
- platform/src/pages/analytics/_components/OverView.jsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- platform/src/common/components/Charts/utils/index.jsx
- platform/src/common/components/Toast/CustomToast.jsx
🔇 Additional comments (10)
platform/src/core/utils/useFetchAnalyticsData.jsx (3)
Line range hint
16-18: LGTM! Clean state managementThe state management is well-structured with appropriate initial values and covers all necessary states (data, loading, error).
Line range hint
61-78: LGTM! Well-structured effect and callback handlingThe dependencies are properly tracked, and the refetch functionality provides good control over data fetching.
41-44: Consider validating request body parametersThe request body construction should include validation for required fields and proper types.
platform/src/pages/analytics/_components/OverView.jsx (1)
209-212: Clean implementation of data integration with charts.The data flow from the hooks to the ChartContainer components is well-structured, with proper handling of loading states, errors, and refresh functionality.
Let's verify the ChartContainer component's prop types:
Also applies to: 220-223
✅ Verification successful
Props match the ChartContainer component's requirements
The verification confirms that all props passed to ChartContainer (
data,chartLoading,error,refetch) are correctly defined in the component's PropTypes and marked as required where appropriate:
data: PropTypes.array.isRequiredchartLoading: PropTypes.bool.isRequirederror: PropTypes.stringrefetch: PropTypes.func.isRequired🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ChartContainer's prop types definition ast-grep --pattern 'ChartContainer.propTypes = { $$$ }'Length of output: 2557
platform/src/common/components/Charts/ChartContainer.jsx (1)
85-87: 🛠️ Refactor suggestionImprove download implementation and error handling
- The current implementation using direct DOM manipulation could be replaced with URL.createObjectURL for cleaner code.
- The CustomToast call doesn't include any parameters, which might limit user feedback.
The previous suggestion about using URL.createObjectURL is still valid.
Consider enhancing the toast notification:
- CustomToast(); + CustomToast({ + message: `Chart exported successfully as ${format.toUpperCase()}`, + type: 'success' + });Also applies to: 102-102
platform/src/common/components/Charts/MoreInsightsChart.jsx (4)
31-31: Nice implementation of responsive width tracking! 👍The integration of
useResizeObserverwithcontainerRefis a clean solution for dynamic width calculations. This approach is more reliable than depending on window resize events.Also applies to: 50-53
162-171: Previous suggestion about magic numbers still applies.
201-201: Verify visual alignment after margin and scale changes.The changes to margin configuration and XAxis scale could affect the chart's visual presentation. Please ensure:
- Data points align correctly with the grid
- Labels don't overlap at various screen sizes
- Chart maintains proper spacing within its container
Consider adding snapshot tests to catch potential visual regressions:
describe('MoreInsightsChart', () => { it('renders correctly with different screen sizes', () => { const { container } = render( <MoreInsightsChart data={mockData} width="100%" height="300px" /> ); expect(container).toMatchSnapshot(); }); });Also applies to: 214-227
332-333: Clean wrapper implementation! ✨The addition of
containerRefand padding creates a well-structured container for the chart.platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (1)
77-78: Verify thatchartData.pollutionTypeprovides the expected value.Ensure that
chartData.pollutionTypeholds the pollutant type in the format expected by your API and other components. This will prevent any issues with data fetching or chart rendering due to incorrect pollutant identifiers.Run the following script to check the values of
pollutionTypein your Redux store:✅ Verification successful
Let me search for the data structure and its usage to understand how pollutionType is handled. The previous command failed because we were looking in the wrong directory. These commands will help us trace the data flow and verify the pollutionType values.
pollutionTypeis correctly implemented and follows expected formatThe verification shows that
pollutionTypeis a well-structured part of the Redux store with proper handling:
- It's initialized with a default value of 'pm2_5' in the ChartSlice
- Accepts only valid values: 'pm2_5', 'pm10', 'no2' (as evidenced by PropTypes)
- Is consistently used across components for:
- Chart rendering with proper WHO standard value mapping
- UI display with proper formatting (PM2.5/PM10)
- Data fetching and measurement value extraction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all occurrences of pollutionType in the codebase to verify its values. rg 'pollutionType' src/ -A 2Length of output: 73
Script:
#!/bin/bash # Let's search from the repository root with correct path rg "pollutionType" -A 2 # Also search for pollutant type related patterns rg "pollutant.*type" -i -A 2 # Look for chart data structure ast-grep --pattern 'const chartData = $_'Length of output: 11378
| // Fetch analytics data for Line Chart | ||
| const { | ||
| allSiteData: lineData, | ||
| chartLoading: lineLoading, | ||
| error: lineError, | ||
| refetch: refetchLine, | ||
| } = useFetchAnalyticsData({ | ||
| selectedSiteIds: chartData.chartSites, | ||
| dateRange: chartData.chartDataRange, | ||
| chartType: 'line', | ||
| frequency: chartData.timeFrame, | ||
| pollutant: chartData.pollutionType, | ||
| organisationName: chartData.organizationName, | ||
| }); | ||
|
|
||
| // Fetch analytics data for Bar Chart | ||
| const { | ||
| allSiteData: barData, | ||
| chartLoading: barLoading, | ||
| error: barError, | ||
| refetch: refetchBar, | ||
| } = useFetchAnalyticsData({ | ||
| selectedSiteIds: chartData.chartSites, | ||
| dateRange: chartData.chartDataRange, | ||
| chartType: 'bar', | ||
| frequency: chartData.timeFrame, | ||
| pollutant: chartData.pollutionType, | ||
| organisationName: chartData.organizationName, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating the duplicate hook calls.
The current implementation makes two separate calls to useFetchAnalyticsData with nearly identical parameters. Consider consolidating these into a single hook call that returns both line and bar data, which could reduce network requests and improve performance.
Here's a suggested approach:
- // Fetch analytics data for Line Chart
- const {
- allSiteData: lineData,
- chartLoading: lineLoading,
- error: lineError,
- refetch: refetchLine,
- } = useFetchAnalyticsData({
- selectedSiteIds: chartData.chartSites,
- dateRange: chartData.chartDataRange,
- chartType: 'line',
- frequency: chartData.timeFrame,
- pollutant: chartData.pollutionType,
- organisationName: chartData.organizationName,
- });
- // Fetch analytics data for Bar Chart
- const {
- allSiteData: barData,
- chartLoading: barLoading,
- error: barError,
- refetch: refetchBar,
- } = useFetchAnalyticsData({
- selectedSiteIds: chartData.chartSites,
- dateRange: chartData.chartDataRange,
- chartType: 'bar',
- frequency: chartData.timeFrame,
- pollutant: chartData.pollutionType,
- organisationName: chartData.organizationName,
- });
+ // Fetch analytics data for both charts
+ const {
+ lineData,
+ barData,
+ loading,
+ error,
+ refetch,
+ } = useFetchAnalyticsData({
+ selectedSiteIds: chartData.chartSites,
+ dateRange: chartData.chartDataRange,
+ frequency: chartData.timeFrame,
+ pollutant: chartData.pollutionType,
+ organisationName: chartData.organizationName,
+ });Committable suggestion was skipped due to low confidence.
| pollutant: chartData.pollutionType, | ||
| organisationName: chartData.organisationName, |
There was a problem hiding this comment.
Ensure consistent pollutant type formatting throughout the component.
There's an inconsistency in how the pollutant type is represented. In the useFetchAnalyticsData hook (lines 77-78), you're using chartData.pollutionType, while in the MoreInsightsChart component (line 254), the pollutantType is hardcoded as "pm2_5". To maintain consistency and prevent potential bugs, consider using chartData.pollutionType throughout the component.
Apply this diff to fix the inconsistency:
<MoreInsightsChart
data={allSiteData}
selectedSites={selectedSiteIds}
chartType={chartType}
frequency={frequency}
width="100%"
height={380}
id="air-quality-chart"
- pollutantType="pm2_5"
+ pollutantType={chartData.pollutionType}
isLoading={chartLoading}
/>Also applies to: 254-254
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 35
🧹 Outside diff range and nitpick comments (22)
platform/src/core/utils/LogoutUser.jsx (1)
5-6: Consider adding success feedback for user experienceWhile the logout logic is correct, consider adding a success toast notification to inform users that they've been logged out successfully. This would align with the new CustomToast component mentioned in the PR objectives.
// Dispatch the RESET_APP action to clear the Redux store dispatch(resetStore()); // Clear local storage to remove any persisted user data if (typeof window !== 'undefined') { localStorage.clear(); + // Show logout success notification + dispatch(showToast({ message: 'Successfully logged out', type: 'success' })); }Also applies to: 8-11
platform/src/core/hooks/useInactivityLogout.jsx (1)
6-45: Consider making the hook more configurableThe hook could be more flexible by accepting configuration options for timeout duration and events to monitor. This would make it more reusable across different contexts.
-const useInactivityLogout = (userId) => { +const useInactivityLogout = (userId, config = {}) => { + const { + inactivityTimeout = 3600000, + checkInterval = 10000, + events = ['mousedown', 'mousemove', 'keydown', 'scroll', 'touchstart'], + onTimeout = () => LogoutUser(dispatch, router) + } = config; // ... rest of the hook implementation using these configured valuesThis would allow consumers to customize the behavior:
useInactivityLogout(userId, { inactivityTimeout: 1800000, // 30 minutes checkInterval: 5000, // 5 seconds events: ['mousedown', 'keydown'], // Custom events onTimeout: () => customLogoutHandler() });platform/src/pages/_app.jsx (1)
Line range hint
50-53: Complete PropTypes validationThe PropTypes validation is missing for the spread operator props.
Add the missing validation:
App.propTypes = { Component: PropTypes.elementType.isRequired, - pageProps: PropTypes.object.isRequired, + pageProps: PropTypes.object, + rest: PropTypes.object };platform/src/pages/analytics/index.jsx (2)
Line range hint
11-21: Consider using a constant for alert state initializationThe alert state initialization could be moved to a constant to improve maintainability and reusability.
+const INITIAL_ALERT_STATE = { type: '', message: '', show: false }; const AuthenticatedHomePage = () => { const dispatch = useDispatch(); - const [alert, setAlert] = useState({ type: '', message: '', show: false }); + const [alert, setAlert] = useState(INITIAL_ALERT_STATE); const [customise, setCustomise] = useState(false);
Line range hint
23-41: Consider extracting chart sites mapping logicThe chart sites mapping logic could be moved to a separate utility function for better maintainability and testability.
+const extractChartSites = (selectedSites) => + selectedSites?.map((site) => site?._id).filter(Boolean) || []; useEffect(() => { if ( preferenceData && preferenceData.length > 0 && preferenceData[0]?.selected_sites ) { - const { selected_sites } = preferenceData[0]; - const chartSites = selected_sites - .map((site) => site?._id) - .filter(Boolean); + const chartSites = extractChartSites(preferenceData[0].selected_sites); dispatch(setChartSites(chartSites)); } }, [dispatch, preferenceData]);platform/src/lib/store/index.js (1)
91-93: Document the wrapper usageThe new wrapper export is a good addition for Next.js integration, but consider adding a brief JSDoc comment explaining its purpose and usage pattern.
+/** + * Next.js Redux wrapper for maintaining store state between pages + * @see https://github.com/kirill-konshin/next-redux-wrapper + */ export const wrapper = createWrapper(makeStore);platform/src/common/components/SideBar/SideBarDrawer.jsx (2)
72-94: Well-structured component extraction with room for improvement.The new
renderCollocationSectionfunction is well-implemented with proper memoization and permission checks. However, consider extracting the permission string to a constants file.Consider this improvement:
+ // In constants/permissions.js + export const PERMISSIONS = { + NETWORK_DEVICES: 'CREATE_UPDATE_AND_DELETE_NETWORK_DEVICES' + }; // In SideBarDrawer.jsx - if (!checkAccess('CREATE_UPDATE_AND_DELETE_NETWORK_DEVICES')) { + if (!checkAccess(PERMISSIONS.NETWORK_DEVICES)) {
Line range hint
97-106: Consider extracting body scroll logic to a custom hook.While the current implementation is correct, this scroll-locking logic could be reused across different modals or drawers.
Consider creating a custom hook:
// hooks/usePreventScroll.js export const usePreventScroll = (shouldPrevent) => { useEffect(() => { if (shouldPrevent) { document.body.style.overflow = 'hidden'; } else { document.body.style.overflow = 'unset'; } return () => { document.body.style.overflow = 'unset'; }; }, [shouldPrevent]); }; // Usage in SideBarDrawer usePreventScroll(togglingDrawer);platform/src/common/components/Dropdowns/OrganizationDropdown.jsx (1)
Line range hint
90-175: Enhance accessibility and improve code readability.The UI implementation needs accessibility improvements and could benefit from better code organization.
Consider these improvements:
- Add ARIA labels and roles:
<CustomDropdown + aria-label="Select organization" trigger={ <Button + aria-expanded={isOpen} paddingStyles="p-0 m-0" className="w-full border-none" variant="outlined" >
- Extract complex conditional classes to constants:
const dropdownButtonClasses = clsx( 'w-full h-12 p-2 bg-white rounded-xl border border-gray-200', { 'flex justify-center': isCollapsed, 'inline-flex justify-between items-center': !isCollapsed, }, 'hover:bg-gray-100' );
- Add keyboard navigation support:
<button key={group._id} onClick={() => handleDropdownSelect(group)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleDropdownSelect(group); + } + }} + role="menuitem" + tabIndex={0}platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
Line range hint
103-146: Enhance error message consistency and user experienceThe error messages in the handleSubmit function could be more consistent and user-friendly. Consider standardizing the error message format and providing more actionable feedback.
Consider these improvements:
if (selectedSites.length === 0) { - setError('No locations selected'); + setError('Please select at least one location to continue'); return; } if (!userID) { - setError('User not found'); + setError('Session expired. Please sign in again'); return; } if (selectedSites.length > 4) { - setError('You can select up to 4 locations only'); + setError('Maximum of 4 locations allowed. Please remove some selections'); return; } // ... rest of the function ... .catch((err) => { - setError('Failed to update preferences'); + setError('Unable to save your preferences. Please try again'); console.error(err); })
Toast integration missing in error handling flow
The component currently uses a local
setErrorstate for error handling, while the rest of the application (including DataDownload.jsx) uses the CustomToast component for user notifications. For consistency, error cases should utilize the toast notification system:
- Replace local error handling with CustomToast in these scenarios:
- No locations selected
- User not found
- Exceeding 4 locations limit
- Failed preferences update
Example pattern from DataDownload.jsx:
import CustomToast from '../../../Toast/CustomToast'; // ... CustomToast(); // For success/error notifications🔗 Analysis chain
Line range hint
103-146: Verify toast integration with data download flowSince this PR introduces download toast functionality, we should verify that the success/error states in this component properly integrate with the new toast system.
Let's check for other toast implementations in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CustomToast usage patterns echo "Searching for CustomToast implementations..." rg -l "CustomToast" # Search for toast-related function calls echo "\nSearching for toast-related function calls..." rg -l "toast\."Length of output: 521
Script:
#!/bin/bash # Let's check how toast is implemented in DataDownload.jsx echo "Checking DataDownload.jsx toast implementation..." rg -A 10 "CustomToast" "platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx" # Let's also check the error handling in AddLocations.jsx echo "\nChecking error handling in AddLocations.jsx..." rg "setError|toast" "platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx" # Let's see if there's a common toast pattern in the codebase echo "\nChecking toast patterns in the codebase..." rg -A 5 "toast\." "reports/src/components/forms/report/ReportForm.tsx"Length of output: 2307
platform/src/pages/account/login/index.jsx (2)
Line range hint
1-18: Consider enhancing token security measures.While the state management consolidation is good, the token handling could be improved by implementing additional security measures.
Consider these security enhancements:
- Implement token expiration checks
- Add HTTP-only cookies for token storage instead of localStorage
- Consider using refresh tokens for better security
Also applies to: 26-27
Line range hint
37-43: Enhance retry logic error categorization.The retry logic correctly handles rate limiting (429), but could be more comprehensive.
Consider this enhancement:
if (retries > 0 && error.response?.status === 429) { await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); return retryWithDelay(fn, retries - 1); +} else if (error.response?.status >= 500) { + // Retry on server errors + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + return retryWithDelay(fn, retries - 1); }platform/src/pages/analytics/_components/OverView.jsx (1)
201-204: Consider adding prop validation for ChartContainer.While the props are correctly passed, adding PropTypes or TypeScript interfaces would improve type safety and documentation.
Example PropTypes implementation:
ChartContainer.propTypes = { data: PropTypes.array.isRequired, chartLoading: PropTypes.bool.isRequired, error: PropTypes.object, refetch: PropTypes.func.isRequired, chartType: PropTypes.oneOf(['line', 'bar']).isRequired, chartTitle: PropTypes.string.isRequired, height: PropTypes.number, id: PropTypes.string.isRequired };Also applies to: 212-215
platform/src/common/components/SideBar/AuthenticatedSidebar.jsx (1)
24-24: Clean up commented code and clarify TODO.Instead of keeping commented code, consider removing it entirely. If the carousel needs to be added back later, this should be tracked in an issue. The TODO comment should provide more context about the conditions or requirements for re-adding this feature.
Would you like me to help create a GitHub issue to track this enhancement?
Also applies to: 203-203
platform/src/common/components/Charts/ChartContainer.jsx (2)
264-278: Remove commented PrintReportModal codeWhile the comment indicates this was retained "as per request", keeping large blocks of commented code is not recommended. This code should be tracked in version control instead of being kept as comments.
Consider removing this commented section entirely. If needed, the code can be retrieved from the git history.
118-118: Enhance toast notification with contextThe current CustomToast call doesn't provide any context about the successful download.
Consider adding a success message:
- CustomToast(); + CustomToast({ + type: 'success', + message: `Chart successfully exported as ${format.toUpperCase()}` + });platform/src/common/components/Charts/MoreInsightsChart.jsx (2)
Line range hint
1-43: Consider adding prop types validation for better type safety.The component accepts multiple props with default values, but lacks type checking. Consider adding PropTypes or migrating to TypeScript for better type safety and documentation.
Example with PropTypes:
import PropTypes from 'prop-types'; MoreInsightsChart.propTypes = { data: PropTypes.array, selectedSites: PropTypes.array, chartType: PropTypes.oneOf(['line', 'bar']), frequency: PropTypes.string, width: PropTypes.string, height: PropTypes.string, id: PropTypes.string, pollutantType: PropTypes.string };
318-320: Enhance chart accessibility.The chart could be more accessible to screen readers and users with color vision deficiencies:
- Add ARIA attributes for screen readers
- Consider using patterns in addition to colors for data series
Example improvements:
-<div id={id} ref={containerRef} className="pt-4"> +<div + id={id} + ref={containerRef} + className="pt-4" + role="img" + aria-label="Air quality measurements chart" +>platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
55-66: Consider adding date range validation.The date range initialization looks good, but it might benefit from validation to ensure the start date isn't after the end date and that the dates are within acceptable bounds for your data availability.
const initialDateRange = useMemo(() => { + const endDate = new Date(); + const startDate = subDays(endDate, 7); + + // Ensure dates are within valid range + if (startDate > endDate) { + console.warn('Invalid date range detected, using default range'); + startDate = subDays(endDate, 1); + } + const { startDateISO, endDateISO } = formatDateRangeToISO( - subDays(new Date(), 7), - new Date(), + startDate, + endDate, ); return { startDate: startDateISO, endDate: endDateISO, label: 'Last 7 days', }; }, []);
Line range hint
188-198: Add date format consistency checks.While the date handling looks functional, consider adding format consistency checks and timezone handling to prevent potential issues.
onChange={(start, end, label) => { if (start && end) { + // Ensure consistent timezone handling + const startUTC = new Date(start.setUTCHours(0, 0, 0, 0)); + const endUTC = new Date(end.setUTCHours(23, 59, 59, 999)); + setDateRange({ - startDate: start.toISOString(), - endDate: end.toISOString(), + startDate: startUTC.toISOString(), + endDate: endUTC.toISOString(), label, }); handleParameterChange(); } }}platform/src/common/components/Layout/index.jsx (1)
82-87: Improve error message presentation for better user experienceDisplaying raw error messages might not offer the best user experience. Consider enhancing the error display by:
- Providing user-friendly messages.
- Using a consistent error handling component across the app.
- Adding accessibility features for screen readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
platform/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
- platform/next.config.js (1 hunks)
- platform/package.json (3 hunks)
- platform/src/common/components/Charts/ChartContainer.jsx (2 hunks)
- platform/src/common/components/Charts/MoreInsightsChart.jsx (2 hunks)
- platform/src/common/components/Dropdowns/CustomDropdown.jsx (2 hunks)
- platform/src/common/components/Dropdowns/OrganizationDropdown.jsx (2 hunks)
- platform/src/common/components/Layout/index.jsx (4 hunks)
- platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2 hunks)
- platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (7 hunks)
- platform/src/common/components/SideBar/AuthenticatedSidebar.jsx (3 hunks)
- platform/src/common/components/SideBar/SideBarDrawer.jsx (5 hunks)
- platform/src/core/hooks/useInactivityLogout.jsx (1 hunks)
- platform/src/core/hooks/useUserChecklists.jsx (1 hunks)
- platform/src/core/hooks/useUserPreferences.jsx (1 hunks)
- platform/src/core/utils/LogoutUser.jsx (1 hunks)
- platform/src/lib/store/index.js (3 hunks)
- platform/src/lib/store/services/account/LoginSlice.js (1 hunks)
- platform/src/pages/_app.jsx (1 hunks)
- platform/src/pages/account/login/index.jsx (8 hunks)
- platform/src/pages/analytics/_components/OverView.jsx (4 hunks)
- platform/src/pages/analytics/index.jsx (1 hunks)
🧰 Additional context used
🪛 Biome
platform/src/common/components/Dropdowns/OrganizationDropdown.jsx
[error] 65-65: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (35)
platform/src/core/utils/LogoutUser.jsx (2)
1-2: LGTM: Clean import statementThe import of
resetStorefrom the login slice is well-structured and follows module import best practices.
13-17: Verify store persistor implementation across the applicationThe optional chaining for store persistor access is a good defensive programming practice. However, let's verify the store implementation across the application to ensure consistency.
✅ Verification successful
Store persistor implementation is consistent across the application
The verification shows a consistent implementation pattern:
- Store persistor is initialized in
platform/src/lib/store/index.js- It's properly accessed with optional chaining in
LogoutUser.jsxfor purging- Used correctly in
_app.jsxfor the PersistGate component🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check store persistor implementation across the codebase # Expected: Consistent usage of store.__persistor # Search for store persistor usage patterns rg -A 2 "__persistor"Length of output: 642
platform/src/lib/store/services/account/LoginSlice.js (3)
17-18: LGTM! Clean and straightforward implementation.
23-24: LGTM! Clean implementation of success state and store reset.The direct return of initialState in resetStore is a good practice for maintaining clean state resets.
Also applies to: 26-26
30-31: LGTM! Exports are properly updated.platform/src/core/hooks/useUserPreferences.jsx (2)
1-4: LGTM! Clean and well-organized imports.
28-28: LGTM! Clean export statement.platform/next.config.js (3)
7-18: Clean and well-structured Next.js configuration.The configuration follows best practices with appropriate settings for images, ESLint, and Docker support.
29-34: Verify the necessity of Node.js module fallbacks.While the fallbacks for
fs,net, andtlswill make the build work, their presence suggests that some dependencies might be using Node.js-specific modules. This could lead to increased bundle size or potential runtime issues in the browser.Let's identify which dependencies are requiring these Node.js modules:
#!/bin/bash # Search for dependencies that might be using Node.js specific modules rg -l "require\(['\"](fs|net|tls)['\"]" --type js --type tsx --type ts # Check package.json dependencies that might be Node.js specific jq -r '.dependencies,.devDependencies|to_entries[]|select(.value)|.key' package.json | xargs npm info --json | jq 'select(.dependencies|has("fs") or has("net") or has("tls"))'
3-6: LGTM! Configuration for redux-persist transpilation looks good.The implementation correctly uses next-transpile-modules to handle redux-persist. The module composition pattern is properly applied.
Let's verify if other modules might need transpilation:
✅ Verification successful
Transpiling redux-persist is necessary and correctly configured
The transpilation of redux-persist is required as it's actively used in the codebase:
- It's a direct dependency in platform/package.json (^6.0.0)
- It's used in the reports app's store configuration for state persistence
- The module is imported in multiple files including store configurations and app initialization
The next-transpile-modules configuration is correctly set up to handle this dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other non-transpiled dependencies that might need similar treatment rg -l "from ['\"]((?!@|\.|\/).+)['\"]" --type js --type tsx --type ts | xargs cat | rg "^import .+ from ['\"]((?!@|\.|\/).+)['\"]"Length of output: 391
Script:
#!/bin/bash # Let's try a different approach to find imports # 1. Find all JS/TS files fd -e js -e jsx -e ts -e tsx | while read -r file; do # 2. Extract imports from these files echo "=== $file ===" rg "^import .+ from ['\"](?![@./])" "$file" || true done # Also check package.json for dependencies that might need transpilation if [ -f "package.json" ]; then echo -e "\n=== Dependencies from package.json ===" cat package.json | jq -r '.dependencies // {} | keys[]' fiLength of output: 567131
Script:
#!/bin/bash # Let's try a simpler approach to find imports # 1. Search for redux-persist imports/requires in all JS files rg -l "require\(['\"]redux-persist['\"]|from ['\"]redux-persist['\"]" --type js --type jsx --type ts --type tsx # 2. Also check package.json for redux-persist usage if [ -f "platform/package.json" ]; then echo -e "\n=== Dependencies from platform/package.json ===" cat platform/package.json | jq -r '.dependencies["redux-persist"] // empty' fiLength of output: 380
Script:
#!/bin/bash # Let's check for redux-persist usage in the codebase # Using grep since rg had issues with file types find . -type f \( -name "*.js" -o -name "*.jsx" -o -name "*.ts" -o -name "*.tsx" \) -exec grep -l "redux-persist" {} \; # Also check for redux store configuration files that might use redux-persist find . -type f \( -name "store.js" -o -name "store.ts" -o -name "store.tsx" -o -name "store.jsx" \) -exec cat {} \;Length of output: 2284
platform/src/core/hooks/useInactivityLogout.jsx (2)
12-14: LGTM! Good use of early return patternThe early return when
userIdis not present prevents unnecessary event listener setup and cleanup.
38-44: LGTM! Proper cleanup implementationThe cleanup logic properly removes all event listeners and clears the interval to prevent memory leaks.
✅ Verification successful
Let me verify if there are any other event listeners in the codebase that might conflict with the cleanup logic in
useInactivityLogout.jsx. I'll search for the specific events being used.
Let me search for all event listeners in the codebase that use these specific events to ensure there are no conflicts.
Clean implementation confirmed - no cleanup conflicts
The cleanup implementation in useInactivityLogout is properly handling its event listeners without conflicts because:
- Most other mousedown listeners in the codebase are scoped to
documentwhile this hook useswindow- The only other mousemove listener is in AppRoutes.js which also properly cleans up its listeners
- All event handlers use unique callback functions, preventing any cross-handler interference
- Every component that adds listeners also implements proper cleanup in their return functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if there are any other instances of event listeners that might conflict rg -A 2 "addEventListener\(.*(?:mousedown|mousemove|keydown|scroll|touchstart)" --type jsLength of output: 174520
Script:
#!/bin/bash # Find the event listeners in useInactivityLogout.jsx rg -A 2 "activityEvents.*=.*\[" platform/src/core/hooks/useInactivityLogout.jsxLength of output: 150
Script:
#!/bin/bash # Search for mousedown and mousemove event listeners in the codebase rg -A 2 "addEventListener\('(mousedown|mousemove)'" --type jsLength of output: 3947
platform/src/pages/analytics/index.jsx (2)
1-9: Clean and well-organized imports! 👌All imports are necessary and properly structured. Good job removing the unused
useMemoimport.
Line range hint
43-60: Verify the implementation of the download toast featureThe PR objectives mention adding a download toast notification, but I don't see its implementation in this component. Please verify if this feature should be added here or if it's implemented elsewhere.
platform/src/core/hooks/useUserChecklists.jsx (4)
1-15: Clean and well-structured hook initialization!The imports, state setup, and Redux integration are well organized. The 5-second debounce for checklist updates provides a good balance between responsiveness and performance.
39-41: Clean effect implementation!The fetch effect is well implemented with proper dependency tracking.
63-66: Clean and standard export implementation!
1-66: Verify Redux integration and component usage.Let's ensure the hook is properly integrated with the rest of the application.
✅ Verification successful
Hook integration is properly implemented and working as expected
The verification shows that the hook is well-integrated:
- Redux actions (
fetchUserChecklists,updateUserChecklists) are properly implemented inCheckData.js- The checklist state management is correctly handled in
CheckList.js- The hook is used in the main Layout component, which ensures checklist data is available throughout the application
- Error handling is properly implemented both in the hook and where it's consumed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Redux action implementations echo "Checking Redux actions..." rg -l "fetchUserChecklists|updateUserChecklists" --type js --type jsx # Check hook usage in components echo "Checking hook usage..." rg -l "useUserChecklists" --type js --type jsx # Check for potential circular dependencies echo "Checking for circular dependencies..." rg -l "@/lib/store/services/checklists" --type js --type jsxLength of output: 464
Script:
#!/bin/bash # Let's try without specifying file types since jsx isn't recognized echo "Checking Redux actions..." rg -l "fetchUserChecklists|updateUserChecklists" echo -e "\nChecking hook usage..." rg -l "useUserChecklists" echo -e "\nChecking for potential circular dependencies..." rg -l "@/lib/store/services/checklists" # Let's also check the store implementation echo -e "\nChecking store implementation..." fd "Check(Data|List).js" --type fLength of output: 1014
Script:
#!/bin/bash # Let's check the actual implementations to verify the integration echo "Checking CheckData.js implementation..." cat platform/src/lib/store/services/checklists/CheckData.js echo -e "\nChecking CheckList.js implementation..." cat platform/src/lib/store/services/checklists/CheckList.js echo -e "\nChecking Layout component usage..." cat platform/src/common/components/Layout/index.jsxLength of output: 7906
platform/package.json (2)
18-18: Verify Redux ecosystem upgrades compatibilityThe major version upgrades in the Redux ecosystem (@reduxjs/toolkit ^2.3.0, react-redux ^9.1.2, redux ^5.0.1) introduce breaking changes that need careful verification:
- Redux v5 removes legacy APIs and adds modern JavaScript features
- Redux Toolkit v2 has significant API changes
- react-redux v9 requires React 18+
Let's verify the Redux usage in the codebase:
#!/bin/bash # Description: Check for potential breaking changes in Redux usage # Check for legacy Redux APIs that might be affected echo "Checking for potentially affected Redux patterns..." rg -l 'createStore|combineReducers|applyMiddleware' # Check for Redux Toolkit usage that might need updates echo "Checking for Redux Toolkit patterns that might need updates..." rg -l 'createAsyncThunk|createSlice|configureStore'Also applies to: 56-56, 60-60
42-42: Verify next-transpile-modules configurationThe addition of [email protected] requires proper configuration in next.config.js to ensure external modules are correctly transpiled.
Let's verify the configuration:
✅ Verification successful
Let me gather more specific information about the next-transpile-modules configuration.
next-transpile-modules is properly configured ✅
The package is correctly set up in next.config.js with
redux-persistmodule being transpiled. The configuration follows the recommended pattern using withTM wrapper:const withTM = require('next-transpile-modules')(['redux-persist']); module.exports = withTM(/* config */);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for next-transpile-modules configuration # Check if next.config.js exists and contains the configuration echo "Checking next.config.js for transpile configuration..." rg -l "withTM|next-transpile-modules" # Look for any module import issues in build logs echo "Checking for any module-related errors in build logs..." rg -l "Failed to compile|Module parse failed"Length of output: 3710
Script:
#!/bin/bash # Check for next.config.js and its content echo "Checking next.config.js files and their content..." fd "next.config.js" --exec cat {} # Check for any withTM usage in JavaScript/TypeScript files echo -e "\nChecking for withTM usage in JS/TS files..." rg "withTM|next-transpile-modules" -A 3 -B 3Length of output: 2783
platform/src/lib/store/index.js (3)
Line range hint
29-51: LGTM! Well-structured reducer combinationThe rootReducer is well-organized with clear naming conventions and proper slice integration. The structure will scale well as new features are added.
1-5: 🛠️ Refactor suggestionConsider consolidating Redux Toolkit usage
You're importing
combineReducersfrom both@reduxjs/toolkitandredux(implicitly through other imports). Since you're already using Redux Toolkit, consider fully embracing it and using its utilities consistently throughout the file.-import { configureStore, combineReducers } from '@reduxjs/toolkit'; +import { configureStore } from '@reduxjs/toolkit';Likely invalid or redundant comment.
53-59: Verify the reset behavior across the applicationThe app reset logic looks good, but we should ensure it properly cleans up all necessary state, especially considering the whitelist in persistConfig.
✅ Verification successful
Let me check the persistConfig to ensure we're handling all persisted state properly.
Let me check how the login and checklists states are being handled since they're in the whitelist.
Reset behavior is properly implemented and comprehensive
The state reset implementation is solid and well-thought-out:
- The
RESET_APPaction properly clears the entire Redux store by setting state to undefinedLogoutUser.jsxhandles both store reset and local storage cleanup- Only
loginandchecklistsstates are persisted (whitelist), and both have proper cleanup:
loginstate is cleared via the main store resetchecklistshas its ownresetChecklistreducer for clean state management🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any potential state cleanup we might miss during reset rg "RESET_APP" -A 5Length of output: 813
Script:
#!/bin/bash # Search for persistConfig to see what state is being persisted rg "persistConfig" -A 10Length of output: 1798
Script:
#!/bin/bash # Look for login and checklists state handling rg "login|checklists" -A 5 "platform/src/lib/store"Length of output: 4471
platform/src/common/components/Dropdowns/CustomDropdown.jsx (2)
80-83: Good addition of event propagation control!The addition of
e.stopPropagation()is a solid improvement that prevents click events from bubbling up, which is especially important in nested UI components like dropdowns within modals or other interactive elements.
Line range hint
80-97: Verify dropdown behavior in nested contexts.The changes to event handling and trigger element behavior warrant verification in complex UI scenarios.
✅ Verification successful
Let me check the specific usage contexts to better understand the potential interaction scenarios.
Let me check one more aspect to ensure we have a complete picture of the event handling in nested contexts.
Event handling implementation is robust and safe for nested contexts
The changes to event handling are well-implemented with proper safeguards:
stopPropagation()inhandleDropdownprevents event bubbling issues- Click-outside handling uses proper element containment checks
- Popper positioning and z-index (1000) ensure proper stacking in modal contexts
- Dropdown state is properly isolated through the
closeDropdowncallbackThe implementation correctly handles:
- Modal + Dropdown nesting in MoreInsights and CustomFields components
- Sidebar collapse states in OrganizationDropdown
- Click-outside detection with proper element boundary checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for nested usage of CustomDropdown within other interactive components rg -l "CustomDropdown.*CustomDropdown|Modal.*CustomDropdown|Drawer.*CustomDropdown" # Look for potential click handler conflicts ast-grep --pattern 'onClick={$_}'Length of output: 499
Script:
#!/bin/bash # Check the implementation in files where CustomDropdown is used within Modal rg -A 10 "CustomDropdown" "platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx" "platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx" # Check OrganizationDropdown implementation for nested dropdown patterns cat "platform/src/common/components/Dropdowns/OrganizationDropdown.jsx" # Look for stopPropagation usage in these contexts rg "stopPropagation" "platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx" "platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx" "platform/src/common/components/Dropdowns/OrganizationDropdown.jsx"Length of output: 16586
Script:
#!/bin/bash # Check for any Modal-specific click handlers that might interact with the dropdown rg -A 5 "Modal.*onClick|onClick.*Modal" platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx # Look for any popper or portal related code that might affect event bubbling rg -A 5 "popper|portal|zIndex" platform/src/common/components/Dropdowns/CustomDropdown.jsxLength of output: 2437
platform/src/common/components/SideBar/SideBarDrawer.jsx (3)
1-1: LGTM! Clean import structure.The direct import of hooks from React follows modern best practices.
115-115: Clean UI structure improvements.The UI changes follow JSX best practices and improve code readability.
Also applies to: 120-120, 154-154
Line range hint
1-177: Verify PR scope and documentation.While the code changes in this file are well-implemented, they seem unrelated to the PR's stated objective of adding a download toast. Please clarify if this refactoring is intentional as part of the PR scope.
platform/src/pages/analytics/_components/OverView.jsx (3)
24-24: LGTM! Clean import statement.The import follows React conventions and is properly placed with other imports.
81-109: Previous review comment about duplicate hook calls is still valid.
225-225: 🛠️ Refactor suggestionVerify the impact of removing React.memo.
The removal of React.memo might affect performance if the component receives frequent prop updates. Consider keeping the memoization unless there's a specific reason for its removal.
Let's check the component's prop update frequency:
Consider keeping the memoization:
-export default OverView; +export default React.memo(OverView);platform/src/common/components/Charts/MoreInsightsChart.jsx (1)
46-49: Verify useResizeObserver hook implementation.The resize observer hook is crucial for responsive behavior. Let's ensure it's properly implemented.
✅ Verification successful
✓ useResizeObserver hook implementation is correct and well-implemented
The hook follows React best practices with proper:
- ResizeObserver setup and cleanup
- Dependency array management
- Null checks for the ref
- Clean type interface returning width and height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of useResizeObserver hook rg -A 10 "useResizeObserver" --type js --type jsx --type ts --type tsxLength of output: 98
Script:
#!/bin/bash # Let's try a different approach to find the hook implementation # First, let's find all possible files that might contain the hook fd -e js -e jsx -e ts -e tsx | xargs rg -l "useResizeObserver" # Then let's look for the actual implementation fd -e js -e jsx -e ts -e tsx | xargs rg -A 10 "export.*useResizeObserver" # Also search for any imports of this hook fd -e js -e jsx -e ts -e tsx | xargs rg "import.*useResizeObserver"Length of output: 955
Script:
#!/bin/bash # Let's examine the implementation of the useResizeObserver hook cat platform/src/core/utils/useResizeObserver.jsLength of output: 811
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (1)
269-269: LGTM! Pollutant type consistency issue resolved.The implementation now correctly uses
chartData.pollutionTypefrom the Redux store, addressing the consistency issue raised in the previous review.platform/src/common/components/Layout/index.jsx (2)
34-34: EnsureuserInfois defined before using inuseInactivityLogoutThe
useInactivityLogouthook is called withuserInfo?._id. IfuserInfois undefined, this could lead to unexpected behavior. Verify thatuserInfois always available when this component renders.
99-99: VerifyModalcomponent receives correct props and functions as intendedEnsure that the
Modalcomponent is properly receiving theisOpenandonCloseprops. This verification will help confirm that the modal opens and closes as expected when theisOpenstate changes and thehandleCloseModalfunction is invoked.
| // Redirect to the login page | ||
| await router.push('/account/login'); | ||
| } catch (error) { | ||
| console.error('Logout failed:', error); | ||
| throw error; // Re-throw the error so it can be handled in the component | ||
| // Optional: Show a notification or feedback to the user | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with specific error types
The current error handling could be more specific. Consider:
- Differentiating between network errors and other types of failures
- Providing appropriate user feedback for different error scenarios
try {
await router.push('/account/login');
} catch (error) {
console.error('Logout failed:', error);
- // Optional: Show a notification or feedback to the user
+ // Show appropriate error notification based on error type
+ const errorMessage = error.message || 'Failed to complete logout process';
+ dispatch(showToast({
+ message: errorMessage,
+ type: 'error'
+ }));
+ // Optionally: Track error for monitoring
+ errorTracker.capture(error);
}Committable suggestion was skipped due to low confidence.
| setError: (state, { payload }) => { | ||
| state.errors = payload; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider normalizing error state structure
To maintain consistent error handling across the application, consider normalizing the error structure.
setError: (state, { payload }) => {
- state.errors = payload;
+ state.errors = payload ? {
+ message: typeof payload === 'string' ? payload : payload.message,
+ code: payload.code || 'UNKNOWN_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.
| setError: (state, { payload }) => { | |
| state.errors = payload; | |
| setError: (state, { payload }) => { | |
| state.errors = payload ? { | |
| message: typeof payload === 'string' ? payload : payload.message, | |
| code: payload.code || 'UNKNOWN_ERROR' | |
| } : null; |
| setUserData: (state, { payload: { key, value } }) => { | ||
| state.userData[key] = value; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding key validation in setUserData reducer
While the dynamic key-value approach is clean, consider adding validation to ensure only valid keys ('userName' and 'password') can be set.
setUserData: (state, { payload: { key, value } }) => {
+ if (!['userName', 'password'].includes(key)) {
+ console.warn(`Invalid key "${key}" provided to setUserData`);
+ return;
+ }
state.userData[key] = value;
},📝 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.
| setUserData: (state, { payload: { key, value } }) => { | |
| state.userData[key] = value; | |
| setUserData: (state, { payload: { key, value } }) => { | |
| if (!['userName', 'password'].includes(key)) { | |
| console.warn(`Invalid key "${key}" provided to setUserData`); | |
| return; | |
| } | |
| state.userData[key] = value; |
| const useUserPreferences = () => { | ||
| const dispatch = useDispatch(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing hook composability and type safety.
The hook could be more useful if it returned the fetched preferences state or a loading indicator.
Consider this enhancement:
-const useUserPreferences = () => {
+const useUserPreferences = () => {
+ const [isLoading, setIsLoading] = useState(false);
const dispatch = useDispatch();
+
+ // Return values for better composability
+ return { isLoading };Committable suggestion was skipped due to low confidence.
| useEffect(() => { | ||
| const fetchPreferences = async () => { | ||
| const storedUser = localStorage.getItem('loggedUser'); | ||
| if (storedUser) { | ||
| try { | ||
| const parsedUser = JSON.parse(storedUser); | ||
| const userId = parsedUser?._id; | ||
| if (userId) { | ||
| await dispatch(getIndividualUserPreferences(userId)); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to parse loggedUser from localStorage:', error); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| fetchPreferences(); | ||
| }, [dispatch]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup and simplify nesting.
The implementation could benefit from race condition prevention and simplified logic.
Consider this enhancement:
useEffect(() => {
+ let isSubscribed = true;
const fetchPreferences = async () => {
+ setIsLoading(true);
const storedUser = localStorage.getItem('loggedUser');
- if (storedUser) {
- try {
- const parsedUser = JSON.parse(storedUser);
- const userId = parsedUser?._id;
- if (userId) {
- await dispatch(getIndividualUserPreferences(userId));
- }
- } catch (error) {
- console.error('Failed to parse loggedUser from localStorage:', error);
- }
+ if (!storedUser) return;
+
+ try {
+ const { _id: userId } = JSON.parse(storedUser);
+ if (userId && isSubscribed) {
+ await dispatch(getIndividualUserPreferences(userId));
+ }
+ } catch (error) {
+ console.error('Failed to parse loggedUser from localStorage:', error);
+ } finally {
+ if (isSubscribed) {
+ setIsLoading(false);
+ }
}
};
fetchPreferences();
+ return () => {
+ isSubscribed = false;
+ };
}, [dispatch]);Committable suggestion was skipped due to low confidence.
| * Unique data keys for plotting, which are site IDs. | ||
| */ | ||
| const dataKeys = useMemo(() => { | ||
| if (chartData.length === 0) return []; | ||
| // Extract all unique keys excluding 'time' | ||
| const keys = new Set(); | ||
| chartData.forEach((item) => { | ||
| Object.keys(item).forEach((key) => { | ||
| if (key !== 'time') keys.add(key); | ||
| }); | ||
| }); | ||
| return Array.from(keys); | ||
| }, [chartData]); | ||
|
|
||
| /** | ||
| * Memoized WHO standard value based on pollutant type | ||
| */ | ||
| const WHO_STANDARD_VALUE = useMemo( | ||
| () => WHO_STANDARD_VALUES[pollutantType] || 0, | ||
| [pollutantType], | ||
| ); | ||
| if (!combinedData[rawTime]) { | ||
| combinedData[rawTime] = { time: rawTime }; | ||
| } | ||
|
|
||
| /** | ||
| * Handler to reset the active index when the mouse leaves a data point. | ||
| */ | ||
| const handleMouseLeave = useCallback(() => setActiveIndex(null), []); | ||
|
|
||
| /** | ||
| * Determines the color of the line or bar based on the active index. | ||
| */ | ||
| const getColor = useCallback( | ||
| (index) => | ||
| activeIndex === null || index === activeIndex | ||
| ? colors[index % colors.length] | ||
| : '#ccc', | ||
| [activeIndex], | ||
| ); | ||
| // Assign value to the corresponding site_id | ||
| combinedData[rawTime][site_id] = value; | ||
| }); | ||
|
|
||
| /** | ||
| * Determines which chart component and data component to use based on the 'chartType' prop. | ||
| */ | ||
| const ChartComponent = chartType === 'line' ? LineChart : BarChart; | ||
| const DataComponent = chartType === 'line' ? Line : Bar; | ||
|
|
||
| /** | ||
| * Calculates the interval for the X-axis ticks based on screen width. | ||
| */ | ||
| const calculateXAxisInterval = useCallback(() => { | ||
| const screenWidth = window.innerWidth; | ||
| if (screenWidth < 768) return Math.ceil(chartData.length / 4); | ||
| if (screenWidth < 1024) return Math.ceil(chartData.length / 6); | ||
| return Math.ceil(chartData.length / 8); | ||
| }, [chartData.length]); | ||
|
|
||
| /** | ||
| * Memoized X-axis interval | ||
| */ | ||
| const xAxisInterval = useMemo( | ||
| () => calculateXAxisInterval(), | ||
| [calculateXAxisInterval], | ||
| // Convert the combined data object to an array and sort it by time | ||
| const sortedData = Object.values(combinedData).sort( | ||
| (a, b) => new Date(a.time) - new Date(b.time), | ||
| ); | ||
|
|
||
| /** | ||
| * Effect to update X-axis interval on window resize for responsiveness | ||
| */ | ||
| useEffect(() => { | ||
| const handleResize = () => { | ||
| // Force recalculation by updating a state or triggering a re-render | ||
| // Here, we do nothing because xAxisInterval is recalculated on dependency change | ||
| }; | ||
| window.addEventListener('resize', handleResize); | ||
| return () => window.removeEventListener('resize', handleResize); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Render the chart or appropriate messages based on state | ||
| */ | ||
| const renderChart = useMemo(() => { | ||
| if (chartData.length === 0 && !isLoading) { | ||
| return ( | ||
| <div className="w-full flex flex-col justify-center items-center h-[380px] text-gray-500"> | ||
| <p className="text-lg font-medium mb-2">No Data Available</p> | ||
| <p className="text-sm"> | ||
| Please select at least one location to view the air quality data. | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return { sortedData, siteIdToName }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in data processing.
The processChartData function handles data transformation but could benefit from additional error handling for malformed data.
Consider wrapping the data processing in a try-catch block:
const processChartData = useCallback((data, selectedSiteIds) => {
+ try {
const combinedData = {};
const siteIdToName = {};
// ... existing code ...
return { sortedData, siteIdToName };
+ } catch (error) {
+ console.error('Error processing chart data:', error);
+ return { sortedData: [], siteIdToName: {} };
+ }
}, []);📝 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 processChartData = useCallback((data, selectedSiteIds) => { | |
| const combinedData = {}; | |
| const siteIdToName = {}; | |
| const allSiteIds = new Set(); | |
| // Build a mapping from site_id to site name | |
| data.forEach((dataPoint) => { | |
| const { site_id, name } = dataPoint; | |
| if (site_id && name) { | |
| siteIdToName[site_id] = name; | |
| allSiteIds.add(site_id); | |
| } | |
| }); | |
| // Only include data points from selected sites | |
| if (!sitesToInclude.includes(site_id)) return; | |
| // If selectedSiteIds is empty, include all site_ids | |
| const sitesToInclude = | |
| selectedSiteIds.length > 0 ? selectedSiteIds : Array.from(allSiteIds); | |
| const rawTime = time; | |
| // Process each data point | |
| data.forEach((dataPoint) => { | |
| const { value, time, site_id } = dataPoint; | |
| if (!combinedData[rawTime]) { | |
| combinedData[rawTime] = { time: rawTime }; | |
| } | |
| // Parse and validate the time using the utility function | |
| const date = parseAndValidateISODate(time); | |
| if (!date) { | |
| return; | |
| } | |
| // Assign value to the corresponding site_id | |
| combinedData[rawTime][site_id] = value; | |
| }); | |
| // Only include data points from selected sites | |
| if (!sitesToInclude.includes(site_id)) return; | |
| // Convert the combined data object to an array and sort it by time | |
| const sortedData = Object.values(combinedData).sort( | |
| (a, b) => new Date(a.time) - new Date(b.time), | |
| ); | |
| const rawTime = time; | |
| return { sortedData, siteIdToName }; | |
| }, []); | |
| /** | |
| * Memoized processed chart data | |
| */ | |
| const { sortedData: chartData, siteIdToName } = useMemo(() => { | |
| if (!data || data.length === 0) | |
| return { sortedData: [], siteIdToName: {} }; | |
| return processChartData(data, selectedSites); | |
| }, [data, selectedSites, processChartData]); | |
| /** | |
| * Unique data keys for plotting, which are site IDs. | |
| */ | |
| const dataKeys = useMemo(() => { | |
| if (chartData.length === 0) return []; | |
| // Extract all unique keys excluding 'time' | |
| const keys = new Set(); | |
| chartData.forEach((item) => { | |
| Object.keys(item).forEach((key) => { | |
| if (key !== 'time') keys.add(key); | |
| }); | |
| }); | |
| return Array.from(keys); | |
| }, [chartData]); | |
| /** | |
| * Memoized WHO standard value based on pollutant type | |
| */ | |
| const WHO_STANDARD_VALUE = useMemo( | |
| () => WHO_STANDARD_VALUES[pollutantType] || 0, | |
| [pollutantType], | |
| ); | |
| if (!combinedData[rawTime]) { | |
| combinedData[rawTime] = { time: rawTime }; | |
| } | |
| /** | |
| * Handler to reset the active index when the mouse leaves a data point. | |
| */ | |
| const handleMouseLeave = useCallback(() => setActiveIndex(null), []); | |
| /** | |
| * Determines the color of the line or bar based on the active index. | |
| */ | |
| const getColor = useCallback( | |
| (index) => | |
| activeIndex === null || index === activeIndex | |
| ? colors[index % colors.length] | |
| : '#ccc', | |
| [activeIndex], | |
| ); | |
| // Assign value to the corresponding site_id | |
| combinedData[rawTime][site_id] = value; | |
| }); | |
| /** | |
| * Determines which chart component and data component to use based on the 'chartType' prop. | |
| */ | |
| const ChartComponent = chartType === 'line' ? LineChart : BarChart; | |
| const DataComponent = chartType === 'line' ? Line : Bar; | |
| /** | |
| * Calculates the interval for the X-axis ticks based on screen width. | |
| */ | |
| const calculateXAxisInterval = useCallback(() => { | |
| const screenWidth = window.innerWidth; | |
| if (screenWidth < 768) return Math.ceil(chartData.length / 4); | |
| if (screenWidth < 1024) return Math.ceil(chartData.length / 6); | |
| return Math.ceil(chartData.length / 8); | |
| }, [chartData.length]); | |
| /** | |
| * Memoized X-axis interval | |
| */ | |
| const xAxisInterval = useMemo( | |
| () => calculateXAxisInterval(), | |
| [calculateXAxisInterval], | |
| // Convert the combined data object to an array and sort it by time | |
| const sortedData = Object.values(combinedData).sort( | |
| (a, b) => new Date(a.time) - new Date(b.time), | |
| ); | |
| /** | |
| * Effect to update X-axis interval on window resize for responsiveness | |
| */ | |
| useEffect(() => { | |
| const handleResize = () => { | |
| // Force recalculation by updating a state or triggering a re-render | |
| // Here, we do nothing because xAxisInterval is recalculated on dependency change | |
| }; | |
| window.addEventListener('resize', handleResize); | |
| return () => window.removeEventListener('resize', handleResize); | |
| }, []); | |
| /** | |
| * Render the chart or appropriate messages based on state | |
| */ | |
| const renderChart = useMemo(() => { | |
| if (chartData.length === 0 && !isLoading) { | |
| return ( | |
| <div className="w-full flex flex-col justify-center items-center h-[380px] text-gray-500"> | |
| <p className="text-lg font-medium mb-2">No Data Available</p> | |
| <p className="text-sm"> | |
| Please select at least one location to view the air quality data. | |
| </p> | |
| </div> | |
| ); | |
| } | |
| return { sortedData, siteIdToName }; | |
| }, []); | |
| const processChartData = useCallback((data, selectedSiteIds) => { | |
| try { | |
| const combinedData = {}; | |
| const siteIdToName = {}; | |
| const allSiteIds = new Set(); | |
| // Build a mapping from site_id to site name | |
| data.forEach((dataPoint) => { | |
| const { site_id, name } = dataPoint; | |
| if (site_id && name) { | |
| siteIdToName[site_id] = name; | |
| allSiteIds.add(site_id); | |
| } | |
| }); | |
| // If selectedSiteIds is empty, include all site_ids | |
| const sitesToInclude = | |
| selectedSiteIds.length > 0 ? selectedSiteIds : Array.from(allSiteIds); | |
| // Process each data point | |
| data.forEach((dataPoint) => { | |
| const { value, time, site_id } = dataPoint; | |
| // Parse and validate the time using the utility function | |
| const date = parseAndValidateISODate(time); | |
| if (!date) { | |
| return; | |
| } | |
| // Only include data points from selected sites | |
| if (!sitesToInclude.includes(site_id)) return; | |
| const rawTime = time; | |
| if (!combinedData[rawTime]) { | |
| combinedData[rawTime] = { time: rawTime }; | |
| } | |
| // Assign value to the corresponding site_id | |
| combinedData[rawTime][site_id] = value; | |
| }); | |
| // Convert the combined data object to an array and sort it by time | |
| const sortedData = Object.values(combinedData).sort( | |
| (a, b) => new Date(a.time) - new Date(b.time), | |
| ); | |
| return { sortedData, siteIdToName }; | |
| } catch (error) { | |
| console.error('Error processing chart data:', error); | |
| return { sortedData: [], siteIdToName: {} }; | |
| } | |
| }, []); |
| if (frequency !== option) { | ||
| setFrequency(option); | ||
| handleParameterChange(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider debouncing parameter changes.
The parameter change handlers look good with their conditional checks, but for better performance, consider debouncing the handleParameterChange calls, especially when dealing with frequent updates.
+import { debounce } from 'lodash';
const MoreInsights = () => {
+ const debouncedParameterChange = useCallback(
+ debounce(() => {
+ refetch();
+ }, 300),
+ [refetch]
+ );
// Replace handleParameterChange with debouncedParameterChange in the onClick handlersAlso applies to: 214-217
| {chartLoading ? ( | ||
| <SkeletonLoader width="100%" height={380} /> | ||
| ) : error ? ( | ||
| <div className="w-full flex items-center justify-center h-[380px]"> | ||
| <p className="text-red-500 font-semibold"> | ||
| Something went wrong. Please try again. | ||
| </p> | ||
| <button | ||
| onClick={refetch} | ||
| className="ml-4 text-red-500 font-semibold underline" | ||
| > | ||
| Try again | ||
| </button> | ||
| </div> | ||
| ) : ( | ||
| <MoreInsightsChart | ||
| data={allSiteData} | ||
| selectedSites={selectedSiteIds} | ||
| chartType={chartType} | ||
| frequency={frequency} | ||
| width="100%" | ||
| height={380} | ||
| id="air-quality-chart" | ||
| pollutantType={chartData.pollutionType} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error feedback for better debugging.
The error handling implementation is good, but consider providing more specific error information to help users and developers understand what went wrong.
- <div className="w-full flex items-center justify-center h-[380px]">
- <p className="text-red-500 font-semibold">
- Something went wrong. Please try again.
+ <div className="w-full flex flex-col items-center justify-center h-[380px]">
+ <p className="text-red-500 font-semibold mb-2">
+ {error.message || 'Something went wrong. Please try again.'}
</p>
+ {process.env.NODE_ENV === 'development' && (
+ <p className="text-sm text-gray-500 mb-2">
+ {error.details || error.stack}
+ </p>
+ )}
<button
onClick={refetch}
- className="ml-4 text-red-500 font-semibold underline"
+ className="px-4 py-2 bg-red-100 text-red-700 rounded-md hover:bg-red-200"
>
Try again
</button>📝 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.
| {chartLoading ? ( | |
| <SkeletonLoader width="100%" height={380} /> | |
| ) : error ? ( | |
| <div className="w-full flex items-center justify-center h-[380px]"> | |
| <p className="text-red-500 font-semibold"> | |
| Something went wrong. Please try again. | |
| </p> | |
| <button | |
| onClick={refetch} | |
| className="ml-4 text-red-500 font-semibold underline" | |
| > | |
| Try again | |
| </button> | |
| </div> | |
| ) : ( | |
| <MoreInsightsChart | |
| data={allSiteData} | |
| selectedSites={selectedSiteIds} | |
| chartType={chartType} | |
| frequency={frequency} | |
| width="100%" | |
| height={380} | |
| id="air-quality-chart" | |
| pollutantType={chartData.pollutionType} | |
| /> | |
| )} | |
| {chartLoading ? ( | |
| <SkeletonLoader width="100%" height={380} /> | |
| ) : error ? ( | |
| <div className="w-full flex flex-col items-center justify-center h-[380px]"> | |
| <p className="text-red-500 font-semibold mb-2"> | |
| {error.message || 'Something went wrong. Please try again.'} | |
| </p> | |
| {process.env.NODE_ENV === 'development' && ( | |
| <p className="text-sm text-gray-500 mb-2"> | |
| {error.details || error.stack} | |
| </p> | |
| )} | |
| <button | |
| onClick={refetch} | |
| className="px-4 py-2 bg-red-100 text-red-700 rounded-md hover:bg-red-200" | |
| > | |
| Try again | |
| </button> | |
| </div> | |
| ) : ( | |
| <MoreInsightsChart | |
| data={allSiteData} | |
| selectedSites={selectedSiteIds} | |
| chartType={chartType} | |
| frequency={frequency} | |
| width="100%" | |
| height={380} | |
| id="air-quality-chart" | |
| pollutantType={chartData.pollutionType} | |
| /> | |
| )} |
| useUserPreferences(); | ||
| useUserChecklists(); |
There was a problem hiding this comment.
Consolidate useUserChecklists hook to prevent redundant calls
The useUserChecklists() hook is called twice: once at line 32 and again at line 40 when destructuring the error. This may lead to unnecessary re-renders or side effects. Consider calling the hook once and using the returned values as needed.
Apply this change to consolidate the hook usage:
- useUserChecklists();
+ const { error } = useUserChecklists();Then, remove the duplicate call at line 40.
Also applies to: 40-40
| }; | ||
|
|
||
| export default Layout; | ||
| export default React.memo(Layout); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider memoizing the Layout component with React.memo
While wrapping Layout with React.memo aims to optimize performance by preventing unnecessary re-renders, it may not have the desired effect if the children prop changes frequently or if the props passed are new on each render. Evaluate if memoization provides a performance benefit in this context.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
CheckCircleIconcomponent for improved visual consistency.CustomToastnotifications for user feedback upon successful data downloads.useResizeObserverhook for dynamic chart resizing based on container dimensions.useFetchAnalyticsDatahook for fetching analytics data in theOverViewcomponent.useInactivityLogout, to log out users after a specified period of inactivity.useUserChecklists, for managing user checklists within a Redux environment.useUserPreferences, for managing user preferences.Enhancements
ChartContainer.MoreInsightsChartto adapt to container size.MoreInsightscomponent.Layoutcomponent by utilizing custom hooks for better separation of concerns.OrganizationDropdowncomponent with improved state management and selection logic.Bug Fixes
MoreInsightsChart.CustomDropdowncomponent to prevent event bubbling.