Analytics UI improvements #2191
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements across multiple components. In 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: 2
🧹 Outside diff range and nitpick comments (6)
platform/src/common/components/AQNumberCard/index.jsx (3)
Line range hint
66-74: Consider enhancing error handling and preventing race conditions.The error handling and loading state management could be improved:
- Error handling: Consider surfacing errors to the user and implementing retry logic.
- Race conditions: Add request cancellation for pending fetches when new ones are triggered.
Here's a suggested implementation:
const fetchMeasurementsForSites = useCallback(async () => { + let isCancelled = false; if (selectedSiteIds.length > 0) { setLoading(true); try { + // Cancel any pending requests here using AbortController await dispatch( fetchRecentMeasurementData({ site_id: selectedSiteIds.join(',') }), ); } catch (error) { - console.error('Error fetching recent measurements:', error); + if (!isCancelled) { + console.error('Error fetching recent measurements:', error); + // Surface error to user through a toast or error boundary + dispatch(setError('Failed to fetch measurements. Please try again.')); + } } finally { - setLoading(false); + if (!isCancelled) { + setLoading(false); + } } } else { setLoading(false); } + return () => { + isCancelled = true; + }; }, [dispatch, selectedSiteIds]);
Line range hint
76-78: Consider cleanup on unmount.The useEffect should handle the cleanup function returned by
fetchMeasurementsForSitesto prevent memory leaks and race conditions.useEffect(() => { - fetchMeasurementsForSites(); + const cleanup = fetchMeasurementsForSites(); + return () => cleanup?.(); }, [fetchMeasurementsForSites]);
Line range hint
14-20: Consider optimizing icon imports.The current approach imports all icons upfront. Consider using dynamic imports to reduce the initial bundle size, especially since these icons are conditionally rendered based on air quality levels.
- import GoodAir from '@/icons/Charts/GoodAir'; - import Hazardous from '@/icons/Charts/Hazardous'; - import Moderate from '@/icons/Charts/Moderate'; - import Unhealthy from '@/icons/Charts/Unhealthy'; - import UnhealthySG from '@/icons/Charts/UnhealthySG'; - import VeryUnhealthy from '@/icons/Charts/VeryUnhealthy'; + const iconMap = { + GoodAir: React.lazy(() => import('@/icons/Charts/GoodAir')), + Hazardous: React.lazy(() => import('@/icons/Charts/Hazardous')), + Moderate: React.lazy(() => import('@/icons/Charts/Moderate')), + Unhealthy: React.lazy(() => import('@/icons/Charts/Unhealthy')), + UnhealthySG: React.lazy(() => import('@/icons/Charts/UnhealthySG')), + VeryUnhealthy: React.lazy(() => import('@/icons/Charts/VeryUnhealthy')), + };platform/src/common/components/Modal/dataDownload/components/DataTable.jsx (2)
262-271: Enhanced search functionality looks good, with a minor suggestion.The addition of new search keys (
search_name,parish,district,sub_county) improves the data discoverability. The search threshold of 0.3 provides a good balance for fuzzy matching.Consider organizing the search keys by category for better maintainability:
keys: [ - 'name', - 'search_name', - 'parish', - 'district', - 'sub_county', - 'city', - 'country', - 'data_provider', + // Name identifiers + 'name', + 'search_name', + // Administrative divisions + 'parish', + 'district', + 'sub_county', + 'city', + 'country', + // Metadata + 'data_provider', ],
262-271: Consider enhancing search accessibility and user feedback.While the search functionality is well-implemented, consider these improvements:
- Add aria-label to the search input to improve accessibility
- Provide more specific feedback when no results match the search criteria
Example implementation:
<TopBarSearch data={uniqueData} onSearch={handleSearch} onClearSearch={handleClearSearch} aria-label="Search locations by name, area, or provider" noResultsMessage="No locations found matching your search criteria" fuseOptions={{ keys: [ // ... existing keys ], threshold: 0.3, }} />platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (1)
Line range hint
300-350: Consider removing commented code and tracking TODOsThere are several commented-out features marked with TODO comments. Instead of keeping commented code in the codebase:
- Track these features in your issue tracker
- Remove the commented code to maintain cleaner files
- Document the planned features in the component's documentation
Would you like me to help create GitHub issues for tracking these pending features?
🧰 Tools
🪛 Biome
[error] 18-18: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- platform/src/common/components/AQNumberCard/index.jsx (1 hunks)
- platform/src/common/components/Modal/dataDownload/components/DataTable.jsx (1 hunks)
- platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (6 hunks)
🧰 Additional context used
🪛 Biome
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx
[error] 18-18: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
platform/src/common/components/AQNumberCard/index.jsx (1)
73-73: Good catch on adding selectedSiteIds to the dependency array! 👍This change ensures that the fetch callback properly reacts to changes in selected sites, maintaining data consistency.
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
27-32: Well-implemented error handling for date validationGood defensive programming practice! The validation logic properly handles invalid dates by logging warnings and skipping problematic entries rather than failing entirely.
180-183: Verify empty onToggle handler implementationThe
onTogglehandler is passed as an empty function. Is this intentional or should it have an implementation? The TODO comments suggest this might be related to pending features.Also applies to: 193-196
| // Validate dateRange | ||
| if (!isValidDate(dateRange.startDate) || !isValidDate(dateRange.endDate)) { | ||
| setError('Invalid date range selected.'); | ||
| setAllSiteData([]); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reset loading state on early validation returns
While the date validation is good, we should reset the loading state when returning early to prevent the UI from being stuck in a loading state.
if (!isValidDate(dateRange.startDate) || !isValidDate(dateRange.endDate)) {
+ setLoading(false);
setError('Invalid date range selected.');
setAllSiteData([]);
return;
}📝 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.
| // Validate dateRange | |
| if (!isValidDate(dateRange.startDate) || !isValidDate(dateRange.endDate)) { | |
| setError('Invalid date range selected.'); | |
| setAllSiteData([]); | |
| return; | |
| } | |
| // Validate dateRange | |
| if (!isValidDate(dateRange.startDate) || !isValidDate(dateRange.endDate)) { | |
| setLoading(false); | |
| setError('Invalid date range selected.'); | |
| setAllSiteData([]); | |
| return; | |
| } |
| // Utility function to validate date | ||
| const isValidDate = (date) => date instanceof Date && !isNaN(date); |
There was a problem hiding this comment.
Use Number.isNaN for safer date validation
The current implementation uses the global isNaN which performs type coercion. Using Number.isNaN is safer and more predictable.
-const isValidDate = (date) => date instanceof Date && !isNaN(date);
+const isValidDate = (date) => date instanceof Date && !Number.isNaN(date.getTime());📝 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.
| // Utility function to validate date | |
| const isValidDate = (date) => date instanceof Date && !isNaN(date); | |
| // Utility function to validate date | |
| const isValidDate = (date) => date instanceof Date && !Number.isNaN(date.getTime()); |
🧰 Tools
🪛 Biome
[error] 18-18: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
platform/src/pages/analytics/_components/OverView.jsx (1)
72-76: Consider removing unused label or implementing its functionality.The dateRange state includes a 'label' property that isn't being used in the component. Either remove it or implement its functionality if it's intended for future use.
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (1)
Line range hint
227-239: Document rationale for disabled featuresSeveral features are commented out with TODOs. Consider adding more context in the comments about:
- Why these features are temporarily disabled
- What conditions need to be met to re-enable them
- Any tracking tickets for their re-implementation
Also applies to: 347-353
🧰 Tools
🪛 Biome
[error] 18-18: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
platform/src/common/components/Calendar/CustomCalendar.jsx (1)
71-71: RenamecomputedValuetodaysDifferencefor clarityRenaming the
computedValuevariable todaysDifferencemakes its purpose clearer, enhancing code readability.Apply this diff to improve clarity:
- const computedValue = Math.abs(differenceInDays(startDate, endDate)); + const daysDifference = Math.abs(differenceInDays(startDate, endDate));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- platform/src/common/components/AQNumberCard/index.jsx (7 hunks)
- platform/src/common/components/Calendar/CustomCalendar.jsx (4 hunks)
- platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (8 hunks)
- platform/src/pages/analytics/_components/OverView.jsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/src/common/components/AQNumberCard/index.jsx
🧰 Additional context used
🪛 Biome
platform/src/common/components/Calendar/CustomCalendar.jsx
[error] 30-30: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx
[error] 18-18: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (4)
platform/src/pages/analytics/_components/OverView.jsx (2)
24-25: LGTM! Good choice of dependencies.Using date-fns for date manipulation is a solid choice, as it's tree-shakeable and lightweight compared to alternatives like moment.js.
Line range hint
72-152: Verify integration with existing analytics functionality.The date range changes look good, but let's verify the integration with existing analytics features.
✅ Verification successful
Let me try a different approach to verify the integration.
Based on the comprehensive search results, I can now provide a definitive response about the integration of the date range changes.
The date range implementation is properly integrated with existing analytics functionality
The search results show that:
- The
chartDataRangeis consistently used across the analytics system, including the chart slice, chart container, and various modal components- The
CustomCalendarcomponent is properly integrated in both analytics and data download features- The date range changes are handled through Redux actions (
setChartDataRange) and properly propagate to all dependent componentsThe implementation maintains consistency with:
- Chart data management (
ChartSlice.js)- Export functionality (
ExportDataModal.jsx,ExportModalWrapper.jsx)- Print report features (
PrintReportModal.jsx)- Analytics data hooks (
useAnalyticsData.jsx)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might be affected by these changes # Look for components using chartDataRange echo "Checking components using chartDataRange..." rg -l "chartDataRange" --type js --type jsx # Look for other uses of CustomCalendar echo "Checking other uses of CustomCalendar..." rg -l "CustomCalendar" --type js --type jsxLength of output: 331
Script:
#!/bin/bash # Search for files using chartDataRange and CustomCalendar without file type restrictions echo "Checking components using chartDataRange..." rg "chartDataRange" echo -e "\nChecking other uses of CustomCalendar..." rg "CustomCalendar" # Also check for any Redux actions related to chart data range echo -e "\nChecking Redux actions for chart data..." rg "setChartDataRange|SET_CHART_DATA_RANGE" # Look for date-related state management echo -e "\nChecking date-related state management..." rg "dateRange|startDate|endDate" --type-not md --type-not jsonLength of output: 43289
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
27-32: Great addition of date validation in data processing!The enhanced error handling with clear warning logs will help debug data quality issues in production.
130-134: Well-structured error handling for unexpected data formatsGood practice throwing specific errors for unexpected data formats. This will help catch API contract changes early.
|
|
||
| <CustomCalendar | ||
| initialStartDate={chartData.chartDataRange.startDate} | ||
| initialEndDate={chartData.chartDataRange.endDate} | ||
| initialStartDate={dateRange.startDate} | ||
| initialEndDate={dateRange.endDate} | ||
| onChange={(start, end) => | ||
| dispatch(setChartDataRange({ start, end })) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and optimize performance.
A few suggestions to improve this section:
- Add error handling for the date range selection
- Debounce the dispatch to prevent unnecessary rerenders
- Consider using CSS classes instead of hardcoded positioning
Here's a suggested implementation:
+import { debounce } from 'lodash';
+
+const debouncedDispatch = debounce((dispatch, start, end) => {
+ try {
+ validateDateRange(start, end);
+ dispatch(setChartDataRange({ start, end }));
+ } catch (error) {
+ // Handle error appropriately
+ console.error(error);
+ }
+}, 300);
<CustomCalendar
initialStartDate={dateRange.startDate}
initialEndDate={dateRange.endDate}
- onChange={(start, end) =>
- dispatch(setChartDataRange({ start, end }))
- }
+ onChange={(start, end) => debouncedDispatch(dispatch, start, end)}
- className="-left-24 md:left-14 lg:left-[70px] top-11"
+ className="calendar-dropdown-position"
dropdown
/>Add to your CSS:
.calendar-dropdown-position {
@apply -left-24 md:left-14 lg:left-[70px] top-11;
}| const [dateRange, setDateRange] = useState({ | ||
| startDate: subDays(new Date(), 7), | ||
| endDate: new Date(), | ||
| label: 'Last 7 days', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date range validation.
Consider adding validation to ensure:
- startDate is not after endDate
- date range doesn't exceed reasonable limits for data fetching
Here's a suggested implementation:
const [dateRange, setDateRange] = useState({
startDate: subDays(new Date(), 7),
endDate: new Date(),
label: 'Last 7 days',
});
+
+const validateDateRange = useCallback((start, end) => {
+ if (start > end) {
+ throw new Error('Start date cannot be after end date');
+ }
+
+ const MAX_RANGE_DAYS = 90;
+ const diffInDays = differenceInDays(end, start);
+ if (diffInDays > MAX_RANGE_DAYS) {
+ throw new Error(`Date range cannot exceed ${MAX_RANGE_DAYS} days`);
+ }
+});Committable suggestion was skipped due to low confidence.
| const MoreInsights = () => { | ||
| const dispatch = useDispatch(); | ||
| const { data: modalData } = useSelector((state) => state.modal.modalType); | ||
| const [chartLoadings, setChartLoadings] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating loading states
There appear to be two loading states: loading and chartLoadings. The commented out setLoading calls suggest this might be a work in progress. Consider consolidating these into a single loading state for better maintainability.
-const [loading, setLoading] = useState(false);
-const [chartLoadings, setChartLoadings] = useState(false);
+const [isLoading, setIsLoading] = useState(false);Also applies to: 124-125, 143-144
| console.warn('Invalid date range selected.'); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback for invalid date selection
Currently, when an invalid date range is selected, a warning is logged to the console, but the user isn't notified. Enhancing user feedback would improve the user experience.
Consider displaying an error message or visual cue in the UI when an invalid date range is selected.
| */ | ||
| const CustomCalendar = ({ initialStartDate, initialEndDate, className }) => { | ||
| const isValidDate = (date) => { | ||
| return date instanceof Date && !isNaN(date); |
There was a problem hiding this comment.
Use Number.isNaN instead of isNaN
Using the global isNaN can lead to unexpected results due to type coercion. It's safer to use Number.isNaN, which doesn't coerce its argument.
Apply this diff to fix the issue:
- return date instanceof Date && !isNaN(date);
+ return date instanceof Date && !Number.isNaN(date);📝 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.
| return date instanceof Date && !isNaN(date); | |
| return date instanceof Date && !Number.isNaN(date); |
🧰 Tools
🪛 Biome
[error] 30-30: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| console.error('Invalid initialStartDate or initialEndDate props.'); | ||
| setValue({ | ||
| startDate: new Date(), | ||
| endDate: new Date(), | ||
| label: 'Invalid Date Range', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle invalid initial dates more gracefully
When initialStartDate or initialEndDate are invalid, the component defaults to the current date and sets the label to 'Invalid Date Range'. This might be confusing for users.
Consider prompting the user to select valid dates instead of defaulting to the current date.
- setValue({
- startDate: new Date(),
- endDate: new Date(),
- label: 'Invalid Date Range',
- });
+ setValue({
+ startDate: null,
+ endDate: null,
+ 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.
| console.error('Invalid initialStartDate or initialEndDate props.'); | |
| setValue({ | |
| startDate: new Date(), | |
| endDate: new Date(), | |
| label: 'Invalid Date Range', | |
| }); | |
| console.error('Invalid initialStartDate or initialEndDate props.'); | |
| setValue({ | |
| startDate: null, | |
| endDate: null, | |
| label: '', | |
| }); |
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
Bug Fixes
These updates enhance user experience by providing more responsive data fetching, broader search capabilities, and improved error handling.