Website: Added Error handling and UI adjustments #2427
Conversation
|
Warning Rate limit exceeded@OchiengPaul442 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces significant refactoring and improvements to the home page components in the website2 project. The changes focus on modularizing code, improving state management, and enhancing component structure across multiple files. Key modifications include creating a new Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
website2/src/views/home/HomeStatsSection.tsx (4)
2-7: Streamline import statements
You appear to be importing some components and hooks from multiple locations. Ensure that any shared imports are consistently declared for better modularity, and consider consolidating them in index files when the codebase grows.
15-29: High-level approach is coherent
TheHomeStatsSectionfunction-level structure is clear, hooking intouseImpactNumbersgracefully. Adding the subcomponents fosters modular design.A small improvement: consider how you handle potential loading or error states from
useImpactNumbersif network latency or an error arises.
32-56: PartnerLogosSection clarity
Rendering partner logos from a data file keeps the component lean. However, consider adding alt text per partner to improve accessibility.- alt={`Partner ${index + 1}`} + alt={partner ? `Partner: ${partner}` : `Partner ${index + 1}`}
122-148: StatisticsSection usage ofimpactNumbers
Your approach to mapping overstatItemsis concise. If the data fromimpactNumbersmight be undefined or of unexpected shape, consider default values or robust checks.website2/src/views/home/HomePlayerSection.tsx (3)
48-75: Memoized TextSection
WrappingTextSectioninReact.memois beneficial for performance. To further optimize, confirm that the props are stable references to avoid unexpected re-renders.
113-179: Modal with AnimatePresence
Neat usage of Framer Motion transitions. Double-check that no additional items on the page become inaccessible while the modal is open for improved accessibility.
249-250: Consistent naming
HomePlayerSection.displayNameis a helpful detail for debugging. Ensure consistency across other memoized or forwardRef components.website2/src/views/home/Accordion.tsx (2)
9-11: AccordionProps structure
Storing items inAccordionPropskeeps the component flexible. In the future, you could extend it to accept custom renderers for advanced layouts.
13-48: Accordion toggle logic
Your toggle approach with a single open item is easy to read. If multi-expand is ever needed, consider arrays foropenItem. The transition classes are also well done.website2/src/views/home/data.ts (2)
13-13: Consider enhancing type safety and accessibility for partner logos.While the array structure is clean, consider using an interface to include additional metadata like alt text for accessibility.
interface PartnerLogo { logo: typeof import('*.svg'); name: string; alt: string; } export const partnerLogos: PartnerLogo[] = [ { logo: Google, name: 'Google', alt: 'Google logo' }, { logo: UsMission, name: 'US Mission', alt: 'US Mission Uganda logo' }, // ... other partners ];
52-83: Standardize key naming convention and add type safety.The statItems array uses inconsistent key naming (e.g.,
african_citiesvs potential camelCase). Let's standardize this and add proper typing.interface StatItem { label: string; key: 'africanCities' | 'champions' | 'deployedMonitors' | 'dataRecords' | 'researchPapers' | 'partners'; icon: typeof import('*.svg'); } export const statItems: StatItem[] = [ { label: 'African Cities', key: 'africanCities', icon: Network, }, // ... other items ];website2/src/views/home/FeaturedCarousel.tsx (2)
Line range hint
69-74: Enhance image loading strategy and accessibility.While the fallback image is a good addition, the image component could benefit from optimized loading attributes and better accessibility.
<Image src={item.image_url || '/placeholder.svg'} alt={item.title} fill className="object-cover w-full h-full transition-transform duration-500 ease-in-out transform hover:scale-110 cursor-pointer" priority + sizes="(max-width: 768px) 100vw, 50vw" + loading={index === currentIndex ? "eager" : "lazy"} + aria-hidden={index !== currentIndex} />
Line range hint
104-134: Enhance carousel accessibility features.The navigation controls are keyboard accessible, but we can improve the overall accessibility of the carousel.
+ // Add at the top of the component + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'ArrowLeft') prevSlide(); + if (e.key === 'ArrowRight') nextSlide(); + }; + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); + }, []); <div className="mt-8 flex items-center gap-4"> + <div className="sr-only" role="status" aria-live="polite"> + Showing slide {currentIndex + 1} of {highlights.length} + </div> <div className="flex items-center gap-2"> <span className="font-medium"> {String(currentIndex + 1).padStart(2, '0')} </span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
website2/src/views/home/Accordion.tsx(1 hunks)website2/src/views/home/FeaturedCarousel.tsx(4 hunks)website2/src/views/home/HomePlayerSection.tsx(2 hunks)website2/src/views/home/HomeStatsSection.tsx(1 hunks)website2/src/views/home/data.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-website2-deploy-preview
🔇 Additional comments (13)
website2/src/views/home/HomeStatsSection.tsx (4)
11-12: Great separation for Accordion and data
SplittingAccordionlogic and data into separate files is an excellent move for maintainability. This improves clarity and reusability.
58-75: HeadingSection tab toggling
The tab switch logic is straightforward, but remember to visually highlight the active tab for accessibility (e.g., ARIA attributes).Would you like a follow-up script to verify proper ARIA usage across the codebase?
78-94: AccordionAndImageSection - single responsibility
Nesting the accordion next to an image is well-executed. If these grow in complexity, consider splitting them to reduce future coupling.
96-120: Dynamic image sources
Switching the image URL based onactiveTabis a nice touch. Ensure these remote images are always valid, or fallback gracefully if the URLs change.website2/src/views/home/HomePlayerSection.tsx (7)
8-8: Optimized import for React
UsinguseCallback,useEffect,useRef, anduseStatefrom React is standard. Make sure nothing is re-exported unnecessarily, to keep the bundle small.
16-27: ForwardRef usage for ReactPlayer
Great strategy for controlling the player and ensuring stable references. This pattern is neat and fosters advanced interactions.
43-46: VideoState interface
Introducing a dedicated interface for modal and video states strengthens readability. Ensure naming is consistent across the codebase.
78-110: VideoSection handles background video
The usage ofvideoRefand a play button overlay is clean. Consider adding a fallback image or handling potential network slowdowns.
180-214: HomePlayerSection state management
The consolidatedvideoStateobject is a good approach. If scale grows, you might consider a more robust store or context to manage cross-component states.
[approve]
215-222: Callbacks for router and dispatch
UsinguseCallbackaroundhandleExploreDataandhandleGetInvolvedis optimal for performance. Great job.
228-242: Conditional modal rendering
The AnimatePresence check forvideoState.isModalOpenis succinct. This ensures dynamic addition/removal doesn’t break the rest of the page.website2/src/views/home/Accordion.tsx (1)
4-7: Clarity in type definitions
Defining a clearAccordionItemtype withtitleandcontentis straightforward and improves maintainability.website2/src/views/home/data.ts (1)
1-12: Well-organized imports with clear grouping!The imports are nicely structured, separating partner logos from impact number icons. Using absolute imports with the
@alias is a good practice for maintainability.
| export const accordionItems = { | ||
| cities: [ | ||
| { | ||
| title: 'High Resolution Network', | ||
| content: | ||
| 'We want cleaner air in all African cities. We leverage our understanding of the African context.', | ||
| }, | ||
| { | ||
| title: 'Digital air quality platforms', | ||
| content: | ||
| 'We empower decision-makers in African cities We increase access to air quality data evidence.', | ||
| }, | ||
| { | ||
| title: 'Policy engagement', | ||
| content: | ||
| 'We engage city authorities and government agencies We empower local leaders with air quality information.', | ||
| }, | ||
| ], | ||
| communities: [ | ||
| { | ||
| title: 'AirQommunity Champions', | ||
| content: | ||
| 'A growing network of individual change makers Championing local leaders and demand action.', | ||
| }, | ||
| { | ||
| title: 'Free Access To Air Quality Information', | ||
| content: | ||
| 'We train individuals and communities Facilitating access to air quality information.', | ||
| }, | ||
| { | ||
| title: 'AirQo Hosts', | ||
| content: | ||
| 'We engage locals host our deployment activities We involve locals in our maintenance drives.', | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve content formatting and add type safety to accordion items.
The content structure needs attention in two areas:
- Content strings contain multiple sentences without proper punctuation
- The object structure lacks type definitions
interface AccordionItem {
title: string;
content: string;
}
interface AccordionCategories {
cities: AccordionItem[];
communities: AccordionItem[];
}
export const accordionItems: AccordionCategories = {
cities: [
{
title: 'High Resolution Network',
content: 'We want cleaner air in all African cities. We leverage our understanding of the African context.',
},
// ... other items
],
// ... other categories
};Also, consider reformatting content strings to use proper punctuation and sentence structure.
| @@ -7,7 +9,7 @@ import mainConfig from '@/configs/mainConfigs'; | |||
| import { useHighlights } from '@/hooks/useApiHooks'; | |||
|
|
|||
| const FeaturedCarousel = () => { | |||
| const { highlights, isLoading, isError } = useHighlights(); | |||
| const { highlights, isLoading } = useHighlights(); | |||
There was a problem hiding this comment.
Restore error handling for better user experience.
The removal of error handling is concerning. When the API call fails, users won't know why the content isn't displaying. Consider implementing a user-friendly error state.
const FeaturedCarousel = () => {
- const { highlights, isLoading } = useHighlights();
+ const { highlights, isLoading, error } = useHighlights();
const [currentIndex, setCurrentIndex] = useState(0);
// ... other code ...
+ if (error) {
+ return (
+ <section className="w-full bg-[#F0F4FA] py-16 md:py-24 overflow-hidden">
+ <div className={`${mainConfig.containerClass} px-4 sm:px-6 lg:px-8`}>
+ <div className="text-center">
+ <p className="text-gray-600">Unable to load highlights. Please try again later.</p>
+ </div>
+ </div>
+ </section>
+ );
+ }
if (!highlights || highlights.length === 0) {
return null;
}Also applies to: 48-48
|
New Website2 changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
website2/src/views/events/EventPage.tsx (1)
Line range hint
18-24: Add null check for airQoEvents array!The component assumes
airQoEventsis always an array, but it could be undefined when the API call hasn't completed or fails. This could lead to runtime errors.Add null checks before filtering:
- const upcomingEvents = airQoEvents.filter( + const upcomingEvents = (airQoEvents || []).filter( (event: any) => new Date(event.end_date) > new Date(), ); - const pastEvents = airQoEvents.filter( + const pastEvents = (airQoEvents || []).filter( (event: any) => new Date(event.end_date) <= new Date(), ); - const featuredEvents = airQoEvents.filter( + const featuredEvents = (airQoEvents || []).filter( (event: any) => event.event_tag === 'Featured', );
♻️ Duplicate comments (1)
website2/src/views/home/FeaturedCarousel.tsx (1)
48-48:⚠️ Potential issueRestore error handling for better user experience.
The removal of error handling is concerning. When the API call fails, users won't know why the content isn't displaying.
const FeaturedCarousel = () => { - const { data: highlights, isLoading } = useHighlights(); + const { data: highlights, isLoading, isError } = useHighlights(); const [currentIndex, setCurrentIndex] = useState(0); // ... other code ... + if (isError) { + return ( + <section className="w-full bg-[#F0F4FA] py-16 md:py-24 overflow-hidden"> + <div className={`${mainConfig.containerClass} px-4 sm:px-6 lg:px-8`}> + <div className="text-center"> + <p className="text-gray-600">Unable to load highlights. Please try again later.</p> + </div> + </div> + </section> + ); + } if (!highlights || highlights.length === 0) { return null; }
🧹 Nitpick comments (11)
website2/src/components/layouts/Navbar.tsx (1)
Line range hint
1-350: Consider these architectural improvements for better maintainability.While the current implementation is solid, here are some suggestions to enhance the codebase:
- Move the
menuItemsconfiguration to a separate file, similar to how you're usingmainConfig. This would make it easier to maintain and reuse.- Consider memoizing the
DropdownMenuContentcomponent and callbacks usinguseMemoanduseCallbackto optimize performance.- Extract the animation logic into a custom hook (e.g.,
useNavbarAnimation) to improve code organization.Would you like me to provide example implementations for any of these suggestions?
website2/src/services/externalService/index.tsx (4)
1-20: Consider enhancing type safety and configuration robustness.The configuration setup is well-structured, but could benefit from the following improvements:
+ // Types for environment variables + type EnvConfig = { + API_URL: string; + API_TOKEN: string; + }; + + // Common headers + const DEFAULT_HEADERS = { + 'Content-Type': 'application/json', + } as const; + - const API_BASE_URL = `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL || '')}/api/v2`; - const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN || ''; + // Validate environment variables + const API_BASE_URL = process.env.NEXT_PUBLIC_API_URL + ? `${removeTrailingSlash(process.env.NEXT_PUBLIC_API_URL)}/api/v2` + : throw new Error('NEXT_PUBLIC_API_URL is not defined'); + + const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN + ?? throw new Error('NEXT_PUBLIC_API_TOKEN is not defined'); const apiClient: AxiosInstance = axios.create({ baseURL: API_BASE_URL, - headers: { - 'Content-Type': 'application/json', - }, + headers: DEFAULT_HEADERS, });
23-59: Enhance request handlers with better type safety and configuration.The generic handlers are well-implemented, but consider these improvements:
+ // Define generic types for API responses + type ApiResponse<T> = { + data: T; + message: string; + status: number; + }; + + // Add request timeout and interceptors + apiClient.interceptors.request.use((config) => { + // Add common headers or tokens if needed + return { + timeout: 5000, // 5 seconds + ...config, + }; + }); + - const getRequest = async (endpoint: string, params?: any): Promise<any | null> + const getRequest = async <T>( + endpoint: string, + params?: Record<string, unknown> + ): Promise<T | null> - const postRequest = async (endpoint: string, body: any): Promise<any | null> + const postRequest = async <T>( + endpoint: string, + body: Record<string, unknown> + ): Promise<T | null>
61-75: Enhance error handling with custom types and user-friendly messages.The error handling is comprehensive, but consider these improvements:
+ // Define custom error types + type ApiError = { + code: string; + message: string; + userMessage: string; + }; + + // Error messages mapping + const ERROR_MESSAGES: Record<string, string> = { + 'network-error': 'Please check your internet connection', + 'not-found': 'The requested resource was not found', + 'server-error': 'Something went wrong on our end', + }; + - const handleError = (error: unknown, context: string) => { + const handleError = (error: unknown, context: string): ApiError => { if (axios.isAxiosError(error)) { const axiosError = error as AxiosError; - console.error(`Error in ${context}:`, axiosError.message); + // Add error tracking (e.g., Sentry) + // captureException(error); + + const code = axiosError.response?.status === 404 ? 'not-found' : 'server-error'; + + return { + code, + message: axiosError.message, + userMessage: ERROR_MESSAGES[code], + }; } else { - console.error(`Unexpected error in ${context}:`, error); + return { + code: 'unknown-error', + message: String(error), + userMessage: 'An unexpected error occurred', + }; } };
77-96: Add type definitions for newsletter and contact form data.The API functions could benefit from proper type definitions:
+ // Define types for request/response data + type NewsletterData = { + email: string; + name?: string; + }; + + type ContactFormData = { + name: string; + email: string; + message: string; + subject: string; + }; + - export const subscribeToNewsletter = async (body: any): Promise<any | null> + export const subscribeToNewsletter = async ( + body: NewsletterData + ): Promise<{ success: boolean; message: string } | null> - export const postContactUs = async (body: any): Promise<any | null> + export const postContactUs = async ( + body: ContactFormData + ): Promise<{ success: boolean; message: string } | null>website2/src/services/apiService/index.tsx (2)
16-26: Consider enhancing error types and handling.While the centralized error handling is a good improvement, consider:
- Creating custom error types for different API failures
- Adding retry logic for transient failures
- Including request metadata in error logs
const getRequest = async (endpoint: string): Promise<any> => { try { const response = await apiClient.get(endpoint); return response.data; } catch (error) { const axiosError = error as AxiosError; - console.error(`Error fetching data from ${endpoint}:`, axiosError.message); + console.error(`Error fetching data from ${endpoint}:`, { + message: axiosError.message, + status: axiosError.response?.status, + data: axiosError.response?.data, + }); throw axiosError; } };
28-114: Consider using specific return types instead ofPromise<any>.While standardizing return types is good, using
anyloses type safety. Consider:
- Creating interfaces for each response type
- Using those interfaces in the return type declarations
Example:
interface PressArticle { id: string; title: string; content: string; // ... other fields } export const getPressArticles = async (): Promise<PressArticle[]> => { return getRequest('/press/'); };website2/src/hooks/useApiHooks.tsx (1)
27-49: Well-structured implementation of the centralized fetch hook!The implementation is clean and follows SWR best practices. Consider adding generic type parameters to improve type safety:
-const useFetch = ( +const useFetch = <T = any>( key: string | null, - fetcher: () => Promise<any>, + fetcher: () => Promise<T>, - initialData: any = [], + initialData: T | null = null, ) => { const { data, error, mutate } = useSWR(key, fetcher, swrOptions); const isLoading = !data && !error; return { - data: data ?? initialData, + data: (data ?? initialData) as T, isLoading, isError: error, mutate, }; };website2/src/views/home/HomeStatsSection.tsx (2)
45-50: Improve image accessibility with descriptive alt text.While the image handling includes good practices like fallback and blend mode, the alt text could be more descriptive than just "Partner X".
<Image src={partner || '/placeholder.svg'} - alt={`Partner ${index + 1}`} + alt={`${partner.split('/').pop()?.split('.')[0]?.replace(/-/g, ' ')} logo`} width={120} height={50} className="mix-blend-multiply w-auto h-auto transition-transform duration-500 ease-in-out transform hover:scale-110 cursor-pointer" />
132-133: Simplify null coalescing with optional chaining.The current implementation can be simplified while maintaining the same functionality.
-{impactNumbers?.[stat.key] ?? 0}+ +{impactNumbers?.[stat.key] || 0}+website2/src/views/about/AboutPage.tsx (1)
38-42: LGTM! Consider enhancing type safety.The explicit destructuring of the
dataproperty aligns well with common data fetching patterns. This change improves code clarity and maintainability.Consider adding TypeScript interfaces for the hook return types to make the data structure more explicit:
interface BoardMember { // add relevant fields } interface UseQueryResult<T> { data: T[]; isLoading: boolean; } const { data: boardMembers, isLoading: loadingBoard }: UseQueryResult<BoardMember> = useBoardMembers();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
website2/src/app/clean-air-forum/layout.tsx(1 hunks)website2/src/app/partners/[id]/page.tsx(1 hunks)website2/src/components/layouts/Navbar.tsx(1 hunks)website2/src/hooks/useApiHooks.tsx(1 hunks)website2/src/services/apiService/index.tsx(1 hunks)website2/src/services/externalService/index.tsx(1 hunks)website2/src/views/about/AboutPage.tsx(1 hunks)website2/src/views/careers/CareerPage.tsx(1 hunks)website2/src/views/careers/DetailsPage.tsx(1 hunks)website2/src/views/cleanairforum/events/EventsPage.tsx(1 hunks)website2/src/views/cleanairforum/membership/MemberPage.tsx(1 hunks)website2/src/views/cleanairforum/resources/ResourcePage.tsx(1 hunks)website2/src/views/events/EventPage.tsx(1 hunks)website2/src/views/events/SingleEvent.tsx(1 hunks)website2/src/views/home/FeaturedCarousel.tsx(4 hunks)website2/src/views/home/HomeStatsSection.tsx(1 hunks)website2/src/views/press/PressPage.tsx(1 hunks)website2/src/views/publications/ResourcePage.tsx(1 hunks)website2/src/views/solutions/AfricanCities/AfricanCities.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-website2-deploy-preview
🔇 Additional comments (16)
website2/src/components/layouts/Navbar.tsx (1)
193-197: Simplify the href value and maintain consistent formatting.Good move on using the configured URL instead of a hardcoded value! However, a few suggestions:
- The template literal is unnecessary here since we're not combining strings. You can directly use
mainConfig.homePageUrl.- The Link props could be formatted more consistently with the rest of the file (single line vs multi-line).
- <Link - href={`${mainConfig.homePageUrl}`} - passHref - className="flex items-center" - > + <Link href={mainConfig.homePageUrl} passHref className="flex items-center">Let's verify the configured home page URL:
✅ Verification successful
Confirmed: Template literal is unnecessary and homePageUrl is correctly configured.
The review comment is spot-on! I've verified that:
mainConfig.homePageUrlis properly defined as '/home' in the configs- This is the only instance of unnecessary template literal usage with
mainConfig- The suggested formatting improvement aligns with the codebase style
The original suggestion to simplify to
href={mainConfig.homePageUrl}is the right way to go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the mainConfig file for the homePageUrl value rg "homePageUrl.*=.*['\"].*['\"]" -g "mainConfigs*"Length of output: 57
Script:
#!/bin/bash # Find files containing mainConfig echo "=== Files containing mainConfig ===" rg -l "mainConfig" --type-add 'config:*.{js,ts,jsx,tsx,json}' -t config echo -e "\n=== mainConfig declarations/imports ===" rg "(?:export|import).*mainConfig" --type-add 'config:*.{js,ts,jsx,tsx,json}' -t config echo -e "\n=== homePageUrl usage ===" rg "homePageUrl" --type-add 'config:*.{js,ts,jsx,tsx,json}' -t configLength of output: 6657
Script:
#!/bin/bash # Check for other similar template literal usage with mainConfig echo "=== Similar template literal usage ===" rg "\`\$\{mainConfig\.[^}]+\}\`" --type-add 'config:*.{js,ts,jsx,tsx}' -t config echo -e "\n=== Other Link href patterns ===" ast-grep --pattern 'href={`$_`}'Length of output: 282
website2/src/app/clean-air-forum/layout.tsx (1)
20-20: Nice standardization of hook destructuring!The change to
data: forumEventsaligns with the codebase-wide standardization of hook return values, improving consistency and maintainability.website2/src/services/apiService/index.tsx (1)
1-14: Good improvement in type safety!The addition of proper type imports and explicit typing of
apiClientasAxiosInstanceenhances type safety and maintainability.website2/src/views/careers/DetailsPage.tsx (1)
13-13: Well-structured component with proper state handling!The hook destructuring change aligns with the standardization effort, and the component demonstrates good practices with comprehensive loading, error, and empty states.
website2/src/app/partners/[id]/page.tsx (1)
13-13: Clean implementation with attention to detail!The hook destructuring change maintains consistency with the codebase standards. The component demonstrates excellent practices:
- Comprehensive state handling
- Responsive skeleton loader
- Proper image optimization with Next.js Image component
- Meaningful alt texts for accessibility
website2/src/hooks/useApiHooks.tsx (1)
90-150: Great organization with clear section separation!The consistent implementation pattern and clear section comments make the code very maintainable.
website2/src/views/cleanairforum/resources/ResourcePage.tsx (1)
44-48: Clean implementation of the new data fetching pattern!The destructuring aligns well with the centralized useFetch implementation.
website2/src/views/publications/ResourcePage.tsx (1)
12-12: Clean refactor of hook destructuring!The change to use
data:prefix in destructuring aligns with standard practices for handling API responses and maintains consistency across the codebase.website2/src/views/cleanairforum/membership/MemberPage.tsx (1)
38-38: LGTM - Hook destructuring refactor!The update to use
data:prefix maintains consistency with the API response structure while preserving the existing categorization logic.website2/src/views/careers/CareerPage.tsx (1)
15-23: Clean hook destructuring updates!Both
useDepartmentsanduseCareershooks have been consistently updated to use thedata:prefix, maintaining a uniform pattern across the application.website2/src/views/events/EventPage.tsx (1)
15-15: Hook destructuring refactor looks good!The update to use
data:prefix aligns with the codebase-wide changes for consistent API response handling.website2/src/views/cleanairforum/events/EventsPage.tsx (1)
49-49: LGTM! Consistent hook data structure.The destructuring pattern aligns with the standardized hook response structure across the codebase.
website2/src/views/events/SingleEvent.tsx (1)
20-20: LGTM! Consistent hook data structure.The destructuring pattern aligns with the standardized hook response structure across the codebase.
website2/src/views/home/FeaturedCarousel.tsx (2)
12-12: LGTM! Consistent hook data structure.The destructuring pattern aligns with the standardized hook response structure across the codebase.
69-69: LGTM! Good fallback for missing images.Adding a fallback image source is a good practice to handle cases where the image URL is undefined.
website2/src/views/home/HomeStatsSection.tsx (1)
15-15: LGTM! Consistent hook data structure.The destructuring pattern aligns with the standardized hook response structure across the codebase.
| /** | ||
| * Fetch grids summary data. Requires API token for authentication. | ||
| */ | ||
| export const getGridsSummary = async (): Promise<any | null> => { | ||
| try { | ||
| const response = await apiClient.get('/devices/grids/summary', { | ||
| params: { | ||
| token: API_TOKEN, | ||
| const response: AxiosResponse<any> = await apiClient.get( | ||
| '/devices/grids/summary', | ||
| { | ||
| params: { | ||
| token: API_TOKEN, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| ); | ||
| return response.data; | ||
| } catch (error) { | ||
| console.error('Error fetching grids summary data:', error); | ||
| handleError(error, 'GET /devices/grids/summary'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor getGridsSummary to use generic handler.
The getGridsSummary function should use the generic getRequest handler for consistency:
+ type GridsSummaryResponse = {
+ total: number;
+ active: number;
+ inactive: number;
+ };
+
- export const getGridsSummary = async (): Promise<any | null> => {
- try {
- const response: AxiosResponse<any> = await apiClient.get(
- '/devices/grids/summary',
- {
- params: {
- token: API_TOKEN,
- },
- },
- );
- return response.data;
- } catch (error) {
- handleError(error, 'GET /devices/grids/summary');
- return null;
- }
- };
+ export const getGridsSummary = async (): Promise<GridsSummaryResponse | null> => {
+ return getRequest<GridsSummaryResponse>('/devices/grids/summary', {
+ token: API_TOKEN,
+ });
+ };Committable suggestion skipped: line range outside the PR's diff.
| export const useEventDetails = (id: string | null) => | ||
| useFetch(id ? `eventDetails/${id}` : null, () => getEventDetails(id!), null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer error handling for ID-based hooks.
The use of the non-null assertion operator (!) with IDs could lead to runtime errors if null is passed. Consider handling the error case explicitly:
export const useEventDetails = (id: string | null) =>
- useFetch(id ? `eventDetails/${id}` : null, () => getEventDetails(id!), null);
+ useFetch(
+ id ? `eventDetails/${id}` : null,
+ () => id ? getEventDetails(id) : Promise.reject(new Error('Invalid ID')),
+ null
+ );Also applies to: 84-89
There was a problem hiding this comment.
reviewed this? @OchiengPaul442
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -9,7 +9,7 @@ import mainConfig from '@/configs/mainConfigs'; | |||
| import { usePressArticles } from '@/hooks/useApiHooks'; | |||
|
|
|||
| const PressPage: React.FC = () => { | |||
| const { pressArticles, isLoading, isError } = usePressArticles(); | |||
| const { data: pressArticles, isLoading, isError } = usePressArticles(); | |||
There was a problem hiding this comment.
Add safety checks for array operations.
The component assumes pressArticles is always an array. Add safety checks to prevent runtime errors:
- const totalPages = Math.ceil(pressArticles.length / itemsPerPage);
+ const totalPages = Math.ceil((pressArticles?.length ?? 0) / itemsPerPage);
- const displayedArticles = pressArticles.slice(
+ const displayedArticles = pressArticles?.slice(
(currentPage - 1) * itemsPerPage,
currentPage * itemsPerPage,
- );
+ ) ?? [];Committable suggestion skipped: line range outside the PR's diff.
| const { data: africanCountries, isLoading, isError } = useAfricanCountries(); | ||
| const [selectedCountry, setSelectedCountry] = useState<any | null>(null); | ||
| const [selectedCity, setSelectedCity] = useState<any | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript interfaces for better type safety.
Replace any types with proper interfaces:
interface City {
id: string;
city_name: string;
content: Array<{
title: string;
description: Array<{ id: string; paragraph: string }>;
image: Array<{ id: string; image_url: string }>;
}>;
}
interface Country {
id: string;
country_name: string;
country_flag_url: string;
city: City[];
}
const [selectedCountry, setSelectedCountry] = useState<Country | null>(null);
const [selectedCity, setSelectedCity] = useState<City | null>(null);
OchiengPaul442
changed the title
|
New Website2 changes available for preview here |
1 similar comment
|
New Website2 changes available for preview here |
|
New Website2 changes available for preview here |
There was a problem hiding this comment.
Thanks @OchiengPaul442 , just highlighted one interested AI bot review comment. Please check it out whenever you get time soon
| export const useEventDetails = (id: string | null) => | ||
| useFetch(id ? `eventDetails/${id}` : null, () => getEventDetails(id!), null); |
There was a problem hiding this comment.
reviewed this? @OchiengPaul442
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
New Features
Refactor
Documentation