Analytics: Code clean up and commenting #2206
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to several components and utilities within the platform. 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 (3)
platform/src/lib/store/services/charts/ChartSlice.js (1)
14-14: Documentation should clarify time zone behavior.The simplified documentation loses important context about time zone handling. This could lead to confusion when the function is used across different time zones.
Consider updating the documentation to be more specific:
- * Calculates the date 7 days prior to today. + * Calculates the date 7 days prior to today using the local time zone. + * Note: This function previously used UTC, but now uses local time.platform/src/pages/analytics/_components/OverView.jsx (2)
40-57: Consider adding error handling to the cleanup effect.The cleanup effect properly resets the chart data range, but it might benefit from error handling since it's dispatching an action.
useEffect(() => { return () => { + try { const { startDate, endDate } = defaultDateRange; const { startDateISO, endDateISO } = formatDateRangeToISO( startDate, endDate, ); dispatch( setChartDataRange({ startDate: startDateISO, endDate: endDateISO, label: defaultDateRange.label, }), ); + } catch (error) { + console.error('Failed to reset chart data range:', error); + } }; }, [dispatch]);
Line range hint
1-250: Consider implementing performance optimizations.The component handles multiple data fetching operations and state updates. Consider these optimizations:
- Memoize the defaultDateRange object
- Use
useMemofor complex calculations in chart data processingExample implementation:
const defaultDateRange = useMemo(() => ({ startDate: subDays(new Date(), 7), endDate: new Date(), label: 'Last 7 days', }), []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- platform/src/common/components/Calendar/CustomCalendar.jsx (2 hunks)
- platform/src/core/utils/useFetchAnalyticsData.jsx (0 hunks)
- platform/src/lib/store/services/charts/ChartSlice.js (1 hunks)
- platform/src/pages/analytics/_components/OverView.jsx (3 hunks)
💤 Files with no reviewable changes (1)
- platform/src/core/utils/useFetchAnalyticsData.jsx
🔇 Additional comments (4)
platform/src/lib/store/services/charts/ChartSlice.js (1)
18-18: Verify the impact of switching from UTC to local time.The change from
setUTCDatetosetDatemeans the function now uses local time instead of UTC. This could affect:
- Data consistency across different time zones
- Chart data alignment
- API interactions expecting specific time formats
Let's check for potential dependencies on UTC timing:
Consider standardizing time zone handling across the application:
- Document the expected time zone behavior
- Use a consistent approach (UTC or local) throughout the codebase
- Consider using a date handling library like
date-fnsormoment-timezonefor more robust time zone management✅ Verification successful
Let me analyze the codebase further to understand the time zone handling patterns.
The previous results show that the codebase uses a mix of UTC and local time handling, with dedicated utilities in
dateTime.js. I need to see how the chart module specifically handles dates to determine if this change is consistent with the module's requirements.
The change from
setUTCDatetosetDateis safe in this contextThe date is immediately converted to ISO string (
toISOString()), which always produces UTC timestamps regardless of whether local or UTC methods were used to manipulate the date. This ensures consistent UTC output for API interactions and data processing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UTC-specific date handling or API calls that might expect UTC rg -i "UTC" --type js --type ts # Look for date-related API calls or data processing ast-grep --pattern 'fetch($$$).then($$$)'Length of output: 19184
Script:
#!/bin/bash # Check for date/time handling in the charts directory rg -A 5 "setDate|setUTCDate|getDate|getUTCDate" platform/src/lib/store/services/charts/ # Look for API calls or data processing in the charts module ast-grep --pattern 'export const $_ = createSlice({ $$$ })' platform/src/lib/store/services/charts/ChartSlice.js # Check for date handling in related utility files rg -A 5 "moment|Date\." platform/src/lib/store/services/charts/Length of output: 1257
platform/src/common/components/Calendar/CustomCalendar.jsx (1)
Line range hint
39-51: Implementation looks good overall!The addition of
initial_labelprop enhances the component's flexibility by allowing external control of the initial label state. The implementation is clean and well-integrated with the existing functionality.platform/src/pages/analytics/_components/OverView.jsx (2)
1-1: LGTM! Clean import organization.The addition of
useEffectimport is properly placed with other React hooks.
38-38: LGTM! Proper state initialization.Good practice using the defaultDateRange for initial state.
| @@ -36,6 +36,7 @@ const isValidDate = (date) => { | |||
| const CustomCalendar = ({ | |||
| initialStartDate, | |||
| initialEndDate, | |||
| initial_label, | |||
There was a problem hiding this comment.
Add PropTypes definition and default value for initial_label.
The new initial_label prop is used in the component but lacks proper type checking and default value definition.
Apply this diff to add the necessary definitions:
CustomCalendar.propTypes = {
initialStartDate: PropTypes.instanceOf(Date).isRequired,
initialEndDate: PropTypes.instanceOf(Date).isRequired,
+ initial_label: PropTypes.string,
onChange: PropTypes.func,
className: PropTypes.string,
dropdown: PropTypes.bool,
isLoading: PropTypes.bool,
};
CustomCalendar.defaultProps = {
+ initial_label: '',
onChange: null,
className: '',
dropdown: false,
isLoading: false,
};Also applies to: 50-50
| startDate: initialStartDate, | ||
| endDate: initialEndDate, | ||
| label: '', | ||
| label: initial_label, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for initial_label in state initialization.
The initial_label prop is used directly in state without validation. Consider adding validation to ensure the label is a non-empty string.
Here's a suggested implementation:
const [value, setValue] = useState({
startDate: initialStartDate,
endDate: initialEndDate,
- label: initial_label,
+ label: typeof initial_label === 'string' ? initial_label : '',
});📝 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.
| startDate: initialStartDate, | |
| endDate: initialEndDate, | |
| label: '', | |
| label: initial_label, | |
| }); | |
| startDate: initialStartDate, | |
| endDate: initialEndDate, | |
| label: typeof initial_label === 'string' ? initial_label : '', | |
| }); |
| // Default date range for the last 7 days | ||
| const defaultDateRange = { | ||
| startDate: subDays(new Date(), 7), | ||
| endDate: new Date(), | ||
| label: 'Last 7 days', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting default date range to constants file.
The defaultDateRange object defines an important default value that might be reused across the analytics module. Consider moving it to a constants file (e.g., lib/constants.js) alongside TIME_OPTIONS and POLLUTANT_OPTIONS.
// In lib/constants.js
+export const DEFAULT_DATE_RANGE = {
+ startDate: subDays(new Date(), 7),
+ endDate: new Date(),
+ label: 'Last 7 days',
+};
// In OverView.jsx
-const defaultDateRange = {
- startDate: subDays(new Date(), 7),
- endDate: new Date(),
- label: 'Last 7 days',
-};
+import { DEFAULT_DATE_RANGE } from '@/lib/constants';
+const defaultDateRange = DEFAULT_DATE_RANGE;Committable suggestion was skipped due to low confidence.
| @@ -142,6 +165,7 @@ | |||
| <CustomCalendar | |||
| initialStartDate={dateRange.startDate} | |||
| initialEndDate={dateRange.endDate} | |||
| initial_label={dateRange.label} | |||
There was a problem hiding this comment.
💡 Codebase verification
The initial_label prop is not defined in CustomCalendar's prop types
Looking at the component's prop types definition, initial_label is being used in the component but is not defined in CustomCalendar.propTypes. This could lead to runtime issues and should be properly typed.
- Add
initial_labelto the prop types inplatform/src/common/components/Calendar/CustomCalendar.jsx:
CustomCalendar.propTypes = {
initialStartDate: PropTypes.instanceOf(Date).isRequired,
initialEndDate: PropTypes.instanceOf(Date).isRequired,
initial_label: PropTypes.string, // Add this
onChange: PropTypes.func,
className: PropTypes.string,
dropdown: PropTypes.bool,
isLoading: PropTypes.bool,
};🔗 Analysis chain
Verify the initial_label prop type in CustomCalendar.
The initial_label prop is being passed to CustomCalendar. Let's verify its prop type definition.
Let me try a different approach to find the component and its prop types definition. I'll search for files containing "CustomCalendar" and then look for its implementation:
Let me also check for TypeScript interface/type definitions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PropTypes definition in CustomCalendar component
rg -A 10 "CustomCalendar.propTypes" src/
Length of output: 85
Script:
#!/bin/bash
# First, let's find the CustomCalendar component file
fd CustomCalendar
# Then search for PropTypes in the found files
rg -A 10 "CustomCalendar.propTypes|type\s+CustomCalendarProps"
Length of output: 1107
Script:
#!/bin/bash
# Search for files containing CustomCalendar
rg -l "CustomCalendar"
# Look for the component definition using ast-grep
ast-grep --pattern 'const CustomCalendar = $_'
Length of output: 20831
Script:
#!/bin/bash
# Search for TypeScript type definitions
rg "interface.*CustomCalendar|type.*CustomCalendar"
Length of output: 53
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
platform/src/common/components/Modal/dataDownload/constants/index.jsx (1)
Line range hint
29-119: Fix duplicate IDs in tableData arrayThe
tableDataarray contains multiple entries with duplicate IDs (5 and 6). This can cause issues with React's key management and state updates.Consider this fix:
{ id: 5, location: 'Nakawa', - }, - { - id: 5, - location: 'Nakawa', - }, - { - id: 5, - location: 'Nakawa', }, { id: 6, location: 'Kansanga', - }, - { - id: 6, - location: 'Kansanga', - }, - { - id: 6, - location: 'Kansanga', },Or better yet, ensure IDs are unique by using incremental values:
{ id: 5, location: 'Nakawa', }, { - id: 5, + id: 7, location: 'Nakawa', }, { - id: 5, + id: 8, location: 'Nakawa', },platform/src/common/components/AQNumberCard/index.jsx (3)
117-122: Nice improvement to loading state visual consistency! 👍The skeleton loader now better matches the actual card design with the addition of the border container. The padding adjustments also create a more accurate representation of the loaded state.
Consider adding a subtle transition to the skeleton animation to match the hover transition of the loaded cards:
- <div className="w-full border border-gray-200 rounded-xl px-4 py-6"> + <div className="w-full border border-gray-200 rounded-xl px-4 py-6 transition-shadow duration-200 ease-in-out">
Line range hint
1-270: Consider these architectural improvements for better maintainability
- Error Handling: The error is only logged to console (line 63). Consider showing a user-friendly error message in the UI.
- Performance: Memoize expensive calculations like
getAirQualityLevelandgetPollutantReading.- Constants: Extract magic numbers/strings (e.g., MAX_CARDS, pollutant types) to named constants.
- Types: Add PropTypes or TypeScript for better type safety.
Would you like me to provide specific code examples for any of these improvements?
Line range hint
29-69: Optimize loading state managementThe current implementation sets loading to true for the entire component during data fetches. Consider implementing per-card loading states to prevent all cards from showing skeletons when only some data is being refreshed.
Here's how you could modify the loading state:
- const [loading, setLoading] = useState(true); + const [loadingStates, setLoadingStates] = useState({}); useEffect(() => { const controller = new AbortController(); if (selectedSiteIds.length > 0) { - setLoading(true); + setLoadingStates(prev => ({ + ...prev, + ...Object.fromEntries(selectedSiteIds.map(id => [id, true])) + })); dispatch( fetchRecentMeasurementsData({ params: { site_id: selectedSiteIds.join(',') }, signal: controller.signal, }), ) .unwrap() .catch((error) => { if (error.name !== 'CanceledError') { console.error('Error fetching recent measurements:', error); } }) .finally(() => { - setLoading(false); + setLoadingStates(prev => ({ + ...prev, + ...Object.fromEntries(selectedSiteIds.map(id => [id, false])) + })); }); } else { - setLoading(false); + setLoadingStates({}); }platform/src/common/components/Charts/ChartContainer.jsx (3)
7-7: Remove commented out code or document the reason.There are several blocks of commented out code related to the PrintReportModal and sharing functionality. If this functionality is temporarily disabled or pending removal, please:
- Remove the code if it's no longer needed
- Add a TODO comment explaining why it's commented out if it's temporary
- Create a tracking issue for re-enabling if planned for future
Also applies to: 173-183, 251-264
Line range hint
89-127: Consider optimizing chart export functionality.The export functionality is well-implemented but could benefit from some improvements:
- The scale factor of 2 might be excessive for large charts
- The JPEG quality of 0.8 could be made configurable
- PDF dimensions could consider page margins
Here's a suggested optimization:
const exportChart = useCallback(async (format) => { if (!chartRef.current) return; setDownloadComplete(null); setLoadingFormat(format); try { + const scale = window.devicePixelRatio || 1; const canvas = await html2canvas(chartRef.current, { - scale: 2, + scale, useCORS: true, backgroundColor: '#fff', }); if (['png', 'jpg'].includes(format)) { + const quality = format === 'jpg' ? 0.8 : 1; - const imgData = canvas.toDataURL(`image/${format}`, 0.8); + const imgData = canvas.toDataURL(`image/${format}`, quality); const link = document.createElement('a'); link.href = imgData; link.download = `airquality-data.${format}`; document.body.appendChild(link); link.click(); document.body.removeChild(link); } else if (format === 'pdf') { + const margin = 40; // 40px margin const pdf = new jsPDF({ orientation: 'landscape', unit: 'px', - format: [canvas.width, canvas.height], + format: [canvas.width + (margin * 2), canvas.height + (margin * 2)], }); const imgData = canvas.toDataURL('image/png', 0.8); - pdf.addImage(imgData, 'PNG', 0, 0, canvas.width, canvas.height); + pdf.addImage(imgData, 'PNG', margin, margin, canvas.width, canvas.height); pdf.save('airquality-data.pdf'); }
Line range hint
54-71: Improve loading state management.The current implementation has a few concerns:
- The TODO comment indicates this is a temporary solution
- The 4-second fixed delay might not be optimal for all scenarios
- The loading state could be more dynamic
Consider implementing a more robust solution:
- // TODO: TEMPORARY TO HANDLE LOADING ISSUE HAVE TO REMOVE LATER useEffect(() => { - let timer; + const minLoadingTime = 1000; // 1 second minimum loading time + const startTime = Date.now(); if (chartLoading) { setShowSkeleton(true); } else { - timer = setTimeout(() => { + const elapsedTime = Date.now() - startTime; + const remainingTime = Math.max(0, minLoadingTime - elapsedTime); + + const timer = setTimeout(() => { setShowSkeleton(false); - }, 4000); + }, remainingTime); + + return () => clearTimeout(timer); } - - return () => { - if (timer) { - clearTimeout(timer); - } - }; }, [chartLoading]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- platform/src/common/components/AQNumberCard/index.jsx (1 hunks)
- platform/src/common/components/Charts/ChartContainer.jsx (1 hunks)
- platform/src/common/components/Charts/MoreInsightsChart.jsx (0 hunks)
- platform/src/common/components/Modal/dataDownload/constants/index.jsx (1 hunks)
💤 Files with no reviewable changes (1)
- platform/src/common/components/Charts/MoreInsightsChart.jsx
🔇 Additional comments (2)
platform/src/common/components/Modal/dataDownload/constants/index.jsx (1)
26-26: Verify the intentional removal of PDF export optionThe PDF option has been commented out rather than removed. Let's verify if this change is intentional and its impact on dependent components.
If PDF export is permanently removed:
- Delete the commented line instead of keeping it
- Update related documentation
- Ensure error handling in components that previously supported PDF export
✅ Verification successful
PDF export option was intentionally removed as part of feature deprecation
The PDF export option was intentionally commented out across the codebase:
- In
ExportDataModal.jsx, PDF is already removed fromexportFormatsarray- In
DataDownload.jsx, while PDF handling code exists, the option is commented out fromFILE_TYPE_OPTIONS- In
PrintReportModal.jsx, there are PDF size limitations (7-day max) and warnings, suggesting a planned deprecationThe removal appears to be part of a deliberate feature deprecation, likely due to the limitations and complexity of PDF generation. The code changes are consistent across the codebase, and the remaining export options (CSV and JSON) are properly maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components that might be affected by PDF export removal rg -l "FILE_TYPE_OPTIONS.*PDF" --type js --type jsxLength of output: 81
Script:
#!/bin/bash # Search for components that might be affected by PDF export removal, using correct file extensions rg -l "FILE_TYPE_OPTIONS.*PDF" -t js -t jsx -t ts -t tsx # Search for any PDF-related export functionality rg -l "export.*PDF" -t js -t jsx -t ts -t tsx # Look for any PDF-related imports or usage rg -l "PDF" -t js -t jsx -t ts -t tsx # Check the content of the constants file cat platform/src/common/components/Modal/dataDownload/constants/index.jsxLength of output: 2039
Script:
#!/bin/bash # Search for files using ripgrep without file type specifications rg -l "FILE_TYPE_OPTIONS" # Search for any PDF-related functionality in the Modal directory rg "PDF" platform/src/common/components/Modal/ -l # Look for components that might use the FILE_TYPE_OPTIONS rg "import.*FILE_TYPE_OPTIONS" platform/src/common/components/ -lLength of output: 548
Script:
#!/bin/bash # Check the content of files that might be affected by PDF removal cat platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx cat platform/src/common/components/Modal/PrintReportModal.jsx cat platform/src/common/components/Modal/ExportDataModal.jsxLength of output: 28809
platform/src/common/components/Charts/ChartContainer.jsx (1)
Line range hint
1-11: Consider maintaining type safety with PropTypes or TypeScript.The removal of PropTypes reduces type safety in the component. While this aligns with the code cleanup objective, it might make the component more prone to runtime errors. Consider either:
- Keeping PropTypes for runtime type checking
- Migrating to TypeScript for better type safety and developer experience
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
initial_labelprop in theCustomCalendarcomponent for better label initialization.OverViewcomponent.MoreInsightsChartcomponent to improve data processing and user experience.Bug Fixes
useFetchAnalyticsDatahook regarding behavior when no site IDs are selected.Documentation
getStartDatefunction in theChartSlice.Improvements
OverViewcomponent to ensure accurate data display.SkeletonCardcomponent in theAQNumberCard.FILE_TYPE_OPTIONSin the data download constants.