[Analytics] Added maintenance banner #2296
Conversation
|
Warning Rate limit exceeded@Codebmk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several updates to the application's layout and maintenance status handling. A new 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
platform/src/core/hooks/useMaintenanceStatus.js (1)
4-4: Consider making the polling interval configurableThe polling interval is hardcoded. Consider making it configurable through props or environment variables for flexibility across different environments.
-const POLLING_INTERVAL = 5 * 60 * 1000; // 5 minutes +const POLLING_INTERVAL = process.env.NEXT_PUBLIC_MAINTENANCE_POLLING_INTERVAL || 5 * 60 * 1000;platform/src/common/components/MaintenanceBanner/index.jsx (1)
1-35: Add unit tests for the componentThe component needs test coverage for various scenarios.
Would you like me to generate unit tests covering:
- Rendering with valid maintenance data
- Not rendering when maintenance is inactive
- Handling invalid dates
- Accessibility checks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
platform/src/common/components/Layout/index.jsx(3 hunks)platform/src/common/components/MaintenanceBanner/index.jsx(1 hunks)platform/src/core/apis/Account.js(2 hunks)platform/src/core/hooks/useMaintenanceStatus.js(1 hunks)platform/src/core/urls/authentication.js(2 hunks)
🔇 Additional comments (5)
platform/src/core/urls/authentication.js (1)
42-42: Consider generalizing the maintenance endpoint
The endpoint /maintenances/analytics seems specific to analytics. Consider whether this maintenance status should be platform-wide rather than analytics-specific.
platform/src/common/components/Layout/index.jsx (3)
12-12: LGTM! Clean import additions
The new imports follow the established path conventions and are logically grouped with similar imports.
Also applies to: 19-19
41-42: LGTM! Proper hook implementation
The maintenance status hook is correctly implemented at the component level, following React hooks best practices.
55-57: Verify banner's visibility across all layout scenarios
The MaintenanceBanner placement looks good, but let's ensure it remains visible across all layout scenarios.
✅ Verification successful
Banner implementation looks good with no visibility concerns
The MaintenanceBanner component uses a simple div structure with Tailwind CSS classes for styling. The implementation:
- Uses fixed positioning relative to its parent container
- Has proper z-index hierarchy through natural DOM order
- Includes clear visual hierarchy with background color and border
- Maintains visibility through contrasting colors (yellow-50 bg with yellow-700 text)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential z-index conflicts and visibility issues
# Search for z-index values in stylesheets and components
rg -n "z-index|zIndex" --type css --type jsx
# Look for other absolute/fixed positioned elements that might overlap
rg -n "position:\s*(fixed|absolute)" --type css --type jsx
Length of output: 163
Script:
#!/bin/bash
# Let's try a different approach to find z-index and positioning styles
# Search in all files since the file extension detection seems problematic
rg -n "z-index|zIndex"
# Search for MaintenanceBanner component definition to check its styling
fd -e js -e jsx -e tsx "MaintenanceBanner" --exec cat {}
# Search for position styles
rg -n "position:\s*(fixed|absolute)"
# Look for styled-components usage with position or z-index
rg -n "styled\." -A 3
Length of output: 100352
Script:
#!/bin/bash
# Let's search for the MaintenanceBanner component definition and its styles
ast-grep --pattern 'const MaintenanceBanner = $$$'
# Search for any styled components related to MaintenanceBanner
rg -l "MaintenanceBanner"
# Look for any CSS modules or style files related to maintenance banner
fd -e css -e scss "maintenance|banner" --exec cat {}
Length of output: 3596
platform/src/core/apis/Account.js (1)
15-15: LGTM! Consistent URL import
The MAINTENANCE_STATUS_URL import follows the established pattern for authentication-related URLs.
| const fetchMaintenanceStatus = async () => { | ||
| try { | ||
| const response = await getMaintenanceStatus(); | ||
| if (response.success && response.maintenance?.length > 0) { | ||
| setMaintenance(response.maintenance[0]); | ||
| } else { | ||
| setMaintenance(null); | ||
| } | ||
| setError(null); | ||
| } catch (err) { | ||
| setError(err); | ||
| console.error('Error fetching maintenance status:', err); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add type validation
The error handling could be more robust:
- No specific error types are being handled
- No validation of response data structure
- Console error in production code
Consider this enhanced implementation:
const fetchMaintenanceStatus = async () => {
try {
const response = await getMaintenanceStatus();
+ // Validate response structure
+ if (!response || typeof response.success !== 'boolean') {
+ throw new Error('Invalid response format');
+ }
if (response.success && response.maintenance?.length > 0) {
+ // Validate maintenance object structure
+ const maintenance = response.maintenance[0];
+ if (!maintenance.startDate || !maintenance.endDate || typeof maintenance.isActive !== 'boolean') {
+ throw new Error('Invalid maintenance data format');
+ }
setMaintenance(response.maintenance[0]);
} else {
setMaintenance(null);
}
setError(null);
} catch (err) {
setError(err);
- console.error('Error fetching maintenance status:', err);
+ // Use a proper logging service in production
+ if (process.env.NODE_ENV === 'development') {
+ console.error('Error fetching maintenance status:', err);
+ }
} finally {
setLoading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchMaintenanceStatus = async () => { | |
| try { | |
| const response = await getMaintenanceStatus(); | |
| if (response.success && response.maintenance?.length > 0) { | |
| setMaintenance(response.maintenance[0]); | |
| } else { | |
| setMaintenance(null); | |
| } | |
| setError(null); | |
| } catch (err) { | |
| setError(err); | |
| console.error('Error fetching maintenance status:', err); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| const fetchMaintenanceStatus = async () => { | |
| try { | |
| const response = await getMaintenanceStatus(); | |
| // Validate response structure | |
| if (!response || typeof response.success !== 'boolean') { | |
| throw new Error('Invalid response format'); | |
| } | |
| if (response.success && response.maintenance?.length > 0) { | |
| // Validate maintenance object structure | |
| const maintenance = response.maintenance[0]; | |
| if (!maintenance.startDate || !maintenance.endDate || typeof maintenance.isActive !== 'boolean') { | |
| throw new Error('Invalid maintenance data format'); | |
| } | |
| setMaintenance(response.maintenance[0]); | |
| } else { | |
| setMaintenance(null); | |
| } | |
| setError(null); | |
| } catch (err) { | |
| setError(err); | |
| // Use a proper logging service in production | |
| if (process.env.NODE_ENV === 'development') { | |
| console.error('Error fetching maintenance status:', err); | |
| } | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
| useEffect(() => { | ||
| fetchMaintenanceStatus(); | ||
| const interval = setInterval(fetchMaintenanceStatus, POLLING_INTERVAL); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for in-flight requests
Use AbortController to clean up pending requests when the component unmounts.
useEffect(() => {
+ const abortController = new AbortController();
+
- fetchMaintenanceStatus();
+ const fetchWithAbort = async () => {
+ try {
+ await fetchMaintenanceStatus({ signal: abortController.signal });
+ } catch (error) {
+ if (!error.name === 'AbortError') {
+ throw error;
+ }
+ }
+ };
+
+ fetchWithAbort();
const interval = setInterval(fetchMaintenanceStatus, POLLING_INTERVAL);
- return () => clearInterval(interval);
+ return () => {
+ clearInterval(interval);
+ abortController.abort();
+ };
}, []);Committable suggestion skipped: line range outside the PR's diff.
| const MaintenanceBanner = ({ maintenance }) => { | ||
| if (!maintenance?.isActive) return null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PropTypes and handle invalid maintenance object
The component lacks prop type validation and could crash with invalid date values.
+import PropTypes from 'prop-types';
+import { isValid } from 'date-fns';
const MaintenanceBanner = ({ maintenance }) => {
- if (!maintenance?.isActive) return null;
+ if (!maintenance?.isActive || !maintenance?.startDate || !maintenance?.endDate) return null;
+
+ const startDate = new Date(maintenance.startDate);
+ const endDate = new Date(maintenance.endDate);
+
+ if (!isValid(startDate) || !isValid(endDate)) {
+ console.warn('Invalid maintenance dates provided');
+ return null;
+ }
+MaintenanceBanner.propTypes = {
+ maintenance: PropTypes.shape({
+ isActive: PropTypes.bool.isRequired,
+ startDate: PropTypes.string.isRequired,
+ endDate: PropTypes.string.isRequired,
+ message: PropTypes.string.isRequired,
+ }),
+};📝 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 MaintenanceBanner = ({ maintenance }) => { | |
| if (!maintenance?.isActive) return null; | |
| import PropTypes from 'prop-types'; | |
| import { isValid } from 'date-fns'; | |
| const MaintenanceBanner = ({ maintenance }) => { | |
| if (!maintenance?.isActive || !maintenance?.startDate || !maintenance?.endDate) return null; | |
| const startDate = new Date(maintenance.startDate); | |
| const endDate = new Date(maintenance.endDate); | |
| if (!isValid(startDate) || !isValid(endDate)) { | |
| console.warn('Invalid maintenance dates provided'); | |
| return null; | |
| } | |
| MaintenanceBanner.propTypes = { | |
| maintenance: PropTypes.shape({ | |
| isActive: PropTypes.bool.isRequired, | |
| startDate: PropTypes.string.isRequired, | |
| endDate: PropTypes.string.isRequired, | |
| message: PropTypes.string.isRequired, | |
| }), | |
| }; |
| return ( | ||
| <div className="bg-yellow-50 border-b border-yellow-200"> | ||
| <div className="max-w-7xl mx-auto py-3 px-3 sm:px-6 lg:px-8"> | ||
| <div className="flex items-center justify-center"> | ||
| <div className="flex items-center"> | ||
| <span className="flex"> | ||
| <IoWarningOutline | ||
| className="h-6 w-6 text-yellow-600" | ||
| aria-hidden="true" | ||
| /> | ||
| </span> | ||
| <p className="ml-3 font-medium text-yellow-700"> | ||
| {maintenance.message} | ||
| <span className="ml-2 text-yellow-600"> | ||
| ({startTime} - {endTime}) | ||
| </span> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience
The banner needs better accessibility support and could benefit from additional UX features.
return (
- <div className="bg-yellow-50 border-b border-yellow-200">
+ <div
+ role="alert"
+ aria-live="polite"
+ className="bg-yellow-50 border-b border-yellow-200"
+ >
<div className="max-w-7xl mx-auto py-3 px-3 sm:px-6 lg:px-8">
<div className="flex items-center justify-center">
<div className="flex items-center">
<span className="flex">
<IoWarningOutline
className="h-6 w-6 text-yellow-600"
- aria-hidden="true"
+ aria-label="Warning"
/>
</span>
- <p className="ml-3 font-medium text-yellow-700">
+ <p className="ml-3 font-medium text-yellow-700" id="maintenance-message">
{maintenance.message}
<span className="ml-2 text-yellow-600">
({startTime} - {endTime})
</span>
</p>
</div>
</div>
</div>
</div>
);📝 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 ( | |
| <div className="bg-yellow-50 border-b border-yellow-200"> | |
| <div className="max-w-7xl mx-auto py-3 px-3 sm:px-6 lg:px-8"> | |
| <div className="flex items-center justify-center"> | |
| <div className="flex items-center"> | |
| <span className="flex"> | |
| <IoWarningOutline | |
| className="h-6 w-6 text-yellow-600" | |
| aria-hidden="true" | |
| /> | |
| </span> | |
| <p className="ml-3 font-medium text-yellow-700"> | |
| {maintenance.message} | |
| <span className="ml-2 text-yellow-600"> | |
| ({startTime} - {endTime}) | |
| </span> | |
| </p> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| return ( | |
| <div | |
| role="alert" | |
| aria-live="polite" | |
| className="bg-yellow-50 border-b border-yellow-200" | |
| > | |
| <div className="max-w-7xl mx-auto py-3 px-3 sm:px-6 lg:px-8"> | |
| <div className="flex items-center justify-center"> | |
| <div className="flex items-center"> | |
| <span className="flex"> | |
| <IoWarningOutline | |
| className="h-6 w-6 text-yellow-600" | |
| aria-label="Warning" | |
| /> | |
| </span> | |
| <p className="ml-3 font-medium text-yellow-700" id="maintenance-message"> | |
| {maintenance.message} | |
| <span className="ml-2 text-yellow-600"> | |
| ({startTime} - {endTime}) | |
| </span> | |
| </p> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| ); |
| export const getMaintenanceStatus = async () => { | ||
| try { | ||
| const response = await createAxiosInstance().get(MAINTENANCE_STATUS_URL); | ||
| return response.data; | ||
| } catch (error) { | ||
| console.error('Error fetching maintenance status:', error); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and reliability
While the basic implementation is functional, consider these improvements:
- Add specific error handling for different failure scenarios
- Implement a retry mechanism for transient failures
- Specify response type for better type safety
export const getMaintenanceStatus = async () => {
try {
- const response = await createAxiosInstance().get(MAINTENANCE_STATUS_URL);
+ const response = await createAxiosInstance().get<MaintenanceStatusResponse>(
+ MAINTENANCE_STATUS_URL,
+ {
+ retry: 3,
+ retryDelay: 1000,
+ }
+ );
return response.data;
} catch (error) {
- console.error('Error fetching maintenance status:', error);
+ if (axios.isAxiosError(error)) {
+ if (error.response?.status === 404) {
+ console.error('Maintenance status endpoint not found');
+ } else if (error.response?.status >= 500) {
+ console.error('Server error while fetching maintenance status');
+ } else {
+ console.error('Network error:', error.message);
+ }
+ }
throw error;
}
};Also, consider defining a type for the maintenance status response:
interface MaintenanceStatusResponse {
active: boolean;
message?: string;
startTime?: string;
endTime?: string;
}
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
platform/src/common/components/MaintenanceBanner/index.jsx (1)
12-34: Add loading and error statesThe component currently doesn't handle loading or error states, which could lead to a poor user experience during API calls or when errors occur.
Consider adding loading and error states:
const MaintenanceBanner = ({ maintenance }) => { + if (maintenance === undefined) { + return ( + <div className="bg-gray-50 border-b border-gray-200 mb-2"> + <div className="py-3 px-3 text-center"> + Loading maintenance status... + </div> + </div> + ); + } + + if (maintenance === null) { + return ( + <div className="bg-red-50 border-b border-red-200 mb-2"> + <div className="py-3 px-3 text-center text-red-700"> + Error loading maintenance status + </div> + </div> + ); + } if (!maintenance?.isActive) return null; // ... rest of the component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
platform/src/common/components/Layout/index.jsx(3 hunks)platform/src/common/components/MaintenanceBanner/index.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/src/common/components/Layout/index.jsx
🔇 Additional comments (1)
platform/src/common/components/MaintenanceBanner/index.jsx (1)
6-8: Enhance input validation
The current null check is insufficient and could lead to runtime errors if the date fields are invalid.
The previous review comment about adding PropTypes and handling invalid maintenance object is still applicable here.
| const startTime = format(new Date(maintenance.startDate), 'HH:mm'); | ||
| const endTime = format(new Date(maintenance.endDate), 'HH:mm'); |
There was a problem hiding this comment.
Implement robust date handling
The current date handling has several potential issues:
- No validation for invalid date strings
- No explicit timezone handling
- No error boundaries for date parsing failures
Consider implementing this more robust approach:
- const startTime = format(new Date(maintenance.startDate), 'HH:mm');
- const endTime = format(new Date(maintenance.endDate), 'HH:mm');
+ const formatMaintenanceTime = (dateString) => {
+ try {
+ const date = new Date(dateString);
+ if (isNaN(date.getTime())) {
+ console.error('Invalid date string:', dateString);
+ return 'Invalid date';
+ }
+ return format(date, 'HH:mm');
+ } catch (error) {
+ console.error('Error formatting date:', error);
+ return 'Invalid date';
+ }
+ };
+
+ const startTime = formatMaintenanceTime(maintenance.startDate);
+ const endTime = formatMaintenanceTime(maintenance.endDate);📝 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 startTime = format(new Date(maintenance.startDate), 'HH:mm'); | |
| const endTime = format(new Date(maintenance.endDate), 'HH:mm'); | |
| const formatMaintenanceTime = (dateString) => { | |
| try { | |
| const date = new Date(dateString); | |
| if (isNaN(date.getTime())) { | |
| console.error('Invalid date string:', dateString); | |
| return 'Invalid date'; | |
| } | |
| return format(date, 'HH:mm'); | |
| } catch (error) { | |
| console.error('Error formatting date:', error); | |
| return 'Invalid date'; | |
| } | |
| }; | |
| const startTime = formatMaintenanceTime(maintenance.startDate); | |
| const endTime = formatMaintenanceTime(maintenance.endDate); |
| import React from 'react'; | ||
| import { IoWarningOutline } from 'react-icons/io5'; | ||
| import { format } from 'date-fns'; | ||
| import dayjs from 'dayjs'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating date manipulation libraries
The component currently uses both date-fns and dayjs for date formatting. This is redundant and increases the bundle size unnecessarily. Consider standardizing on one library, preferably date-fns since it's already being used for the basic time formatting.
import React from 'react';
import { IoWarningOutline } from 'react-icons/io5';
import { format } from 'date-fns';
-import dayjs from 'dayjs';Committable suggestion skipped: line range outside the PR's diff.
| ); | ||
| }; | ||
|
|
||
| export default MaintenanceBanner; |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for MaintenanceBanner component
The component is currently used in the Layout component and serves a clear purpose, but lacks test coverage. A test file should be added to ensure the component's reliability, especially since it handles maintenance state display.
- Add test file at
platform/src/common/components/MaintenanceBanner/MaintenanceBanner.test.jsx - Include test cases for:
- Rendering with maintenance prop
- Proper display of maintenance message
- Component behavior when maintenance prop is empty/false
🔗 Analysis chain
Verify component usage and add tests
Let's ensure the component is properly integrated and tested:
- Verify all usage locations
- Add unit tests for the component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MaintenanceBanner usage
echo "Searching for MaintenanceBanner usage..."
rg "MaintenanceBanner" --type jsx --type tsx
# Check for existing test file
echo "Checking for test file..."
fd "MaintenanceBanner.test" --type file
Length of output: 264
Script:
#!/bin/bash
# Search for MaintenanceBanner usage with correct file extensions
echo "Searching for MaintenanceBanner usage..."
rg "MaintenanceBanner" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx"
# Check for test files in a more comprehensive way
echo "Checking for test files..."
fd -e test.js -e test.jsx -e spec.js -e spec.jsx "MaintenanceBanner"
# Look for the component definition to understand its location
echo "Finding component definition..."
rg "class MaintenanceBanner|function MaintenanceBanner|const MaintenanceBanner" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx"
Length of output: 1001
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
This pull request introduces a new feature to display a maintenance banner on the platform. The changes include adding a new
MaintenanceBannercomponent, creating a hook to fetch maintenance status, and updating relevant files to incorporate these changes.New Feature: Maintenance Banner
platform/src/common/components/Layout/index.jsx: AddedMaintenanceBannerimport and usage in theLayoutcomponent to display the maintenance banner if maintenance is active. [1] [2] [3]platform/src/common/components/MaintenanceBanner/index.jsx: Created a newMaintenanceBannercomponent that displays a warning message with the maintenance schedule.API Integration
platform/src/core/apis/Account.js: Added a new API functiongetMaintenanceStatusto fetch the maintenance status from the server.platform/src/core/urls/authentication.js: Defined a new URL constantMAINTENANCE_STATUS_URLfor the maintenance status endpoint.Hook for Maintenance Status
platform/src/core/hooks/useMaintenanceStatus.js: Created a new hookuseMaintenanceStatusto poll the maintenance status at regular intervals and provide the status to components.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
New Features
MaintenanceBannercomponent to display maintenance notifications.useMaintenanceStatus, to manage and retrieve maintenance status.getMaintenanceStatus, to fetch maintenance status from the server.Bug Fixes
Documentation