Add Donation Receipt functionality and UI improvements #2416
Conversation
…footer sections for donation receipt page
…s with translations
…hen downloadUrl is available.
…ameters (dtn, year, challenge) - Implemented logic to fetch donation receipt data when valid query parameters are present. - Added type checks for dtn, year, and challenge before making the API request. - Updated the donation receipt data upon successful fetch. - Handled errors in case of failed API request.
…ontact support section
- added next-intl keys for "donationAmount" and "name" with currency and type-based rendering. - defined proper types for Donor, Donation, and ReceiptData for stricter TypeScript typing. - moved fetchReceiptData logic from ReceiptDataSection to DonationReceiptLayout for better separation of concerns. - simplified UI by removing firstName and lastName fields, displaying only Name. - added skeleton structure for the receipt verification page to enhance loading experience.
- added cursor pointer style to RedirectArrowButton for better usability. - Implemented the `modify-user-data` route.
- rename `donation-receipts` to `donation-receipt`. - use `useCallback` for the routing function to optimize performance.
…ents - added `formatReceiptData` helper to transform `ReceiptDataAPI` into `ReceiptData` with strict type validation. - Updated `ReceiptData` to include `operation` and `issuedDonations` fields. - Improved type safety for `DonorView`, `AddressView`, and donation-related properties. - Refactored components to align with the updated ReceiptData structure and context logic.
- Rename hasUserDataChanged to hasDonorDataChanged
…ataSection - Relocated the `confirmDonorData` logic to improve component structure and ensure relevant logic resides within `ReceiptDataSection`. -Introduce spinner for loading state.
![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 (9)
src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx (3)
39-47: Consider disabling the button when processing instead of using an empty click handler.Currently, when
isProcessingis true, the button is still clickable but has an empty function handler. It would be better to disable the button entirely during processing to provide clearer feedback to users.<WebappButton variant="primary" elementType="button" text={!isProcessing ? tReceipt('verifyAndDownload') : undefined} - onClick={!isProcessing ? onReceiptClick : () => {}} + onClick={onReceiptClick} + disabled={isProcessing} icon={isProcessing ? <div className={styles.spinner} /> : undefined} buttonClasses={styles.receiptCardButton} />
34-34: Add error handling for formatDate function.The
formatDatefunction is called directly withpaymentDatewithout any error handling. IfpaymentDateis invalid or undefined, this might cause runtime errors.- date={formatDate(paymentDate)} + date={paymentDate ? formatDate(paymentDate) : ''}
42-45: Improve accessibility for processing state.When the button is in a processing state, it's not clear to screen readers what's happening. Consider adding an aria-label to improve accessibility.
<WebappButton variant="primary" elementType="button" text={!isProcessing ? tReceipt('verifyAndDownload') : undefined} onClick={!isProcessing ? onReceiptClick : () => {}} icon={isProcessing ? <div className={styles.spinner} /> : undefined} + aria-label={isProcessing ? tReceipt('processing') : tReceipt('verifyAndDownload')} buttonClasses={styles.receiptCardButton} />src/features/user/DonationReceipt/DonationReceiptWrapper.tsx (1)
106-115: Provide user-friendly error feedback.Currently, an error only logs to the console. Consider offering more structured error handling (e.g., showing an error message in the UI) so users know what's happening.
} catch (error) { console.error('❌ Error during receipt operation:', error); + // Optionally show a user-facing error message + // setErrors([{ message: 'An unexpected error occurred while processing your receipt.' }]); } finally { setIsLoading(false); }src/features/common/Layout/DonationReceiptContext.tsx (3)
77-86: Enhance error handling when loading from session storage.The current implementation catches errors and logs them to the console, but consider adding more robust error handling that could notify users or trigger a recovery mechanism.
const loadStateFromSession = (): DonationReceiptContextState => { try { const storedState = sessionStorage.getItem('donationReceiptContext'); return storedState ? JSON.parse(storedState) : defaultState; } catch (error) { console.error('Failed to parse session storage:', error); sessionStorage.removeItem('donationReceiptContext'); + // Consider adding analytics or user notification for persistent errors + // Optionally trigger recovery mechanism if appropriate return defaultState; } };
29-33: Ensure responsive design for all viewport sizes.The current layout adjusts for extra-small phone views, but consider adding media queries for other common breakpoints (tablet, larger phones) to ensure a smooth experience across all device sizes.
134-136: Consider adding a cleanup mechanism on component unmount.Currently, session storage is cleared manually via the
clearSessionStoragemethod, but consider adding a cleanup mechanism using a useEffect with an empty dependency array to clear storage when the provider unmounts in certain scenarios.export const DonationReceiptProvider: React.FC<{ children: React.ReactNode; }> = ({ children }) => { const [state, setState] = useState<DonationReceiptContextState>(loadStateFromSession); // Persist state to sessionStorage useEffect(() => { sessionStorage.setItem('donationReceiptContext', JSON.stringify(state)); }, [state]); + // Optional: Clean up on unmount if needed in specific scenarios + // useEffect(() => { + // return () => { + // // Only clear in specific scenarios, not on every unmount + // // Add logic to determine when to clear + // // if (shouldClearOnUnmount) { + // // sessionStorage.removeItem('donationReceiptContext'); + // // } + // }; + // }, []); //clean donation receipt data from the session storage const clearSessionStorage = (): void => { sessionStorage.removeItem('donationReceiptContext'); };src/features/user/DonationReceipt/DonationReceipt.module.scss (2)
401-402: Add a method to handle scrollbar visibility across browsers.While you've hidden scrollbars in Firefox and IE/Edge, Chrome requires a different approach. Consider adding the following to ensure consistent scrollbar hiding across all browsers.
.donationReceipts { max-width: 524px; max-height: 494px; overflow-y: auto; width: 100%; display: flex; flex-direction: column; gap: 21px; margin-top: 21px; scrollbar-width: none; /* Hides scrollbar in Firefox */ -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ + &::-webkit-scrollbar { + display: none; /* Hides scrollbar in Chrome/Safari */ + } }
489-515: Consider using CSS variables for animation properties.For better maintainability, consider defining the animation properties as CSS variables. This would make it easier to adjust timing and behavior across multiple elements.
+ :root { + --spinner-animation-duration: 1s; + --spinner-size: 16px; + --spinner-border-width: 2px; + } .spinner { border: 2px solid #f3f3f3; border-radius: 50%; border-top: 2px solid transparent; - width: 16px; - height: 16px; + width: var(--spinner-size); + height: var(--spinner-size); - -webkit-animation: spin 1s linear infinite; /* Safari */ - animation: spin 1s linear infinite; + -webkit-animation: spin var(--spinner-animation-duration) linear infinite; /* Safari */ + animation: spin var(--spinner-animation-duration) linear infinite; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/donationReceipt.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
public/assets/images/icons/projectV2/NoDataFound.tsx(2 hunks)src/features/common/Layout/DonationReceiptContext.tsx(1 hunks)src/features/projectsV2/ProjectList/index.tsx(2 hunks)src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceiptValidator.ts(1 hunks)src/features/user/DonationReceipt/DonationReceiptWrapper.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceipts.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- public/assets/images/icons/projectV2/NoDataFound.tsx
- src/features/projectsV2/ProjectList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/user/DonationReceipt/DonationReceipts.tsx
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/user/DonationReceipt/DonationReceipt.module.scss
[warning] 363-363: src/features/user/DonationReceipt/DonationReceipt.module.scss#L363
Unexpected shorthand "margin" after "margin-top". (declaration-block-no-shorthand-property-overrides)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx (4)
25-26: Add null check for donations array before accessing elements.The code directly accesses the last element of the
donationsarray without checking if the array is empty. This could cause a runtime error ifdonationsis empty.- const reference = donations[donations.length - 1].uid; + const reference = donations.length > 0 ? donations[donations.length - 1].uid : '';
1-9: LGTM - Well-structured imports.The imports are well-organized, separating type imports from regular imports and properly importing required components and utilities.
10-14: LGTM - Clear prop type definition.The component's prop type is well-defined with appropriate types for each prop.
28-56: LGTM - Clean component structure with conditional rendering.The component is well-structured with appropriate conditional rendering based on the receipt type. The use of internationalization for text is a good practice for supporting multiple languages.
src/features/user/DonationReceipt/DonationReceiptWrapper.tsx (2)
38-40: Add missing dependency touseEffect.Your
clearSessionStoragefunction is not included in the dependency array, which can lead to stale references if the function is recreated.
94-105: Consider adding a fallback for unrecognized operation.The current logic handles
VERIFYandISSUEbut does nothing if the operation has a different status. For robustness, you might add anelsebranch with a warning or error to handle unexpected values.src/features/user/DonationReceipt/DonationReceiptValidator.ts (1)
24-31: Verify logic for returningtruewhen user is not logged in.
validateOwnershipgrants ownership if there is no logged-in user. Ensure this is the intended behavior, as it allows unauthenticated access ifissuedToEmailis not validated.src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx (2)
33-33: Add null/empty check fordonationsarray.Accessing
donations[donations.length - 1]can cause a runtime error ifdonationsis empty.
47-55: Better handle potential null or undefineddownloadUrl.Using an empty string fallback (
downloadUrl ?? '') can create broken links. Consider conditionally rendering the link only ifdownloadUrlis present or providing a safer fallback.src/features/user/DonationReceipt/DonationReceipt.module.scss (1)
436-436: LGTM: Good responsive design patterns.The stylesheet makes effective use of media queries with the
@include xsPhoneViewmixin to adapt layouts for smaller screens, which is crucial for the mobile responsiveness mentioned in the PR objectives.
| // TODO: review the following validation | ||
| return donor !== null && |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO comment about validation.
There's a comment indicating that the validation logic needs to be reviewed. Consider implementing more comprehensive validation that checks not only for null values but also validates the format and content of the data.
🏁 Script executed:
#!/bin/bash
# Find if there are other validation implementations in the codebase that could be reused
echo "Searching for validation patterns in the codebase..."
rg -A 3 -B 3 "validate.*Receipt|receipt.*validate" --type tsx --type tsLength of output: 216
Below is a new shell script that uses a glob pattern to include both TypeScript and TSX files, which should help us verify whether any reusable receipt validation logic exists:
Once you run this command, please check if any reusable validation implementations are found. If no such patterns exist, then the review comment still holds: the current check only verifies that donor is not null, without ensuring that the donor data’s format or content is correct.
Based on the current evidence—and pending further verification—the rewritten review comment is as follows:
Action Required: Enhance Donation Receipt Validation
In src/features/common/Layout/DonationReceiptContext.tsx (lines 280–281), the donor validation only checks for a non-null value:
// TODO: review the following validation
return donor !== null &&
This minimal check does not confirm that the donor data is properly formatted or contains valid content. Since no reusable validation utilities for receipt data appear to exist in the codebase, please consider one of the following:
- Extend this validation: Implement additional checks to ensure the data meets expected format and content criteria.
- Refactor if needed: If similar validation logic is required elsewhere, abstract it into a reusable utility function.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Re-run search for validation patterns in TS and TSX files using globbing
rg -g '*.{ts,tsx}' -A 3 -B 3 "validate.*Receipt|receipt.*validate"Length of output: 4936
Action Required: Replace Minimal Donor Check with Robust Validation
In src/features/common/Layout/DonationReceiptContext.tsx (lines 280–281), the current check:
// TODO: review the following validation
return donor !== null &&only ensures that donor is not null. However, similar parts of the codebase already leverage more comprehensive receipt validations via the functions validateIssuedReceipt and validateUnissuedReceipt (located in src/features/user/DonationReceipt/DonationReceiptValidator.ts). To avoid duplication and ensure consistent validation across the application, please consider one of the following approaches:
- Refactor to use existing validators: Replace the inline
donor !== nullcheck with a call to eithervalidateIssuedReceiptorvalidateUnissuedReceipt(depending on context) so that both the format and content of the donor data are properly validated. - Extend the current validation: If a more tailored validation is needed in this context, augment the check to include necessary format and data integrity verification beyond simply confirming a non-null value.
Please update this logic accordingly.
| justify-content: space-between; | ||
| gap: 12px; | ||
| margin-top: 68px; | ||
| margin: 42px 12px 0px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix conflicting margin declarations.
The margin-top property is being overridden by the subsequent margin shorthand property, making the first declaration ineffective.
- margin-top: 68px;
margin: 42px 12px 0px;📝 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.
| margin: 42px 12px 0px; | |
| margin: 42px 12px 0px; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 363-363: src/features/user/DonationReceipt/DonationReceipt.module.scss#L363
Unexpected shorthand "margin" after "margin-top". (declaration-block-no-shorthand-property-overrides)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx (3)
46-51: Consider simplifying the condition for invalid address.The current check for invalid address fields requires each field to be evaluated separately. This can be simplified for better readability and maintainability.
isAddressInvalid={ - !address.address1 || - !address.zipCode || - !address.city || - !address.country + !address.address1 || !address.zipCode || !address.city || !address.country }
53-59: Enhance the login alert with ActionButton component.The login alert simply informs the user that login is required but doesn't provide a direct way to login.
Consider adding an ActionButton component to allow users to login directly from this notification:
{!isAuthenticated && ( <p className={styles.loginAlert}> {tReceipt.rich('notifications.loginRequired', { b: (chunks) => <strong>{chunks}</strong>, })} + <ActionButton className={styles.loginButton} onClick={() => loginWithRedirect()}> + {tReceipt('actions.login')} + </ActionButton> </p> )}This would require importing the necessary components and hooks:
import { useAuth0 } from '@auth0/auth0-react'; import ActionButton from '../../../../common/InputTypes/ActionButton'; // In the component: const { isAuthenticated, loginWithRedirect } = useAuth0();
61-71: Improve loading state with more descriptive feedback.The current loading state shows only a spinner without any context about what's happening.
Consider adding a loading message to provide better user feedback:
) : ( <div className={styles.donationReceiptSpinner}> <CircularProgress color="success" /> + <p className={styles.loadingMessage}>{tReceipt('messages.processingReceipt')}</p> </div> )}You'll need to add the corresponding style in the SCSS file:
.loadingMessage { margin-top: 10px; font-size: $fontSmall; color: rgba(130, 130, 130, 1); }And add a translation key for the loading message in your locale files.
src/features/user/DonationReceipt/DonationReceipt.module.scss (2)
133-136: Use CSS variables for consistent colors.The background color is hardcoded as an rgba value, which makes it difficult to maintain consistent theming across the application.
.donorDetails { display: flex; flex-direction: column; gap: 20px; - background: rgba(242, 242, 242, 0.5); + background: var(--background-light); width: 100%; border-radius: 9px; padding: 10px 15px;Define this variable in your theme file if it doesn't exist yet.
488-496: Replace custom spinner with MUI's CircularProgress.You've defined a custom spinner when the application is already using MUI's CircularProgress component elsewhere in the codebase (as seen in ReceiptDataSection.tsx).
Consider replacing this custom spinner implementation with the consistent use of MUI's CircularProgress component to maintain visual consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
public/static/locales/cs/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/de/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/en/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/es/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/fr/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/it/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/pt-BR/donationReceipt.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceipts.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationsTable.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/user/DonationReceipt/DonationReceipts.tsx
- src/features/user/DonationReceipt/microComponents/DonationsTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
| <button | ||
| type="button" | ||
| onClick={openPopover} | ||
| className={styles.donationCount} | ||
| > | ||
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | ||
| reference, | ||
| count, | ||
| date, | ||
| u: (chunks) => <span>{chunks}</span>, | ||
| })} | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for the donation count button.
The button lacks proper accessibility attributes, which could make it difficult for screen reader users to understand its purpose.
<button
type="button"
onClick={openPopover}
className={styles.donationCount}
+ aria-label={tReceipt('aria.showDonationDetails')}
+ aria-haspopup="true"
+ aria-expanded={Boolean(popoverAnchor)}
>
{tReceipt.rich('itemsReferenceDateMultiDonation', {
reference,
count,
date,
u: (chunks) => <span>{chunks}</span>,
})}
</button>This requires adding a new translation key for the aria-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.
| <button | |
| type="button" | |
| onClick={openPopover} | |
| className={styles.donationCount} | |
| > | |
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | |
| reference, | |
| count, | |
| date, | |
| u: (chunks) => <span>{chunks}</span>, | |
| })} | |
| </button> | |
| )} | |
| <button | |
| type="button" | |
| onClick={openPopover} | |
| className={styles.donationCount} | |
| aria-label={tReceipt('aria.showDonationDetails')} | |
| aria-haspopup="true" | |
| aria-expanded={Boolean(popoverAnchor)} | |
| > | |
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | |
| reference, | |
| count, | |
| date, | |
| u: (chunks) => <span>{chunks}</span>, | |
| })} | |
| </button> | |
| )} |
| {tReceipt('donationDetails.donationAmount', { | ||
| currency, | ||
| amount: amount.toFixed(2), | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for amount formatting.
The amount.toFixed(2) call assumes amount is always a valid number. This could potentially cause runtime errors if amount is null, undefined, or NaN.
{tReceipt('donationDetails.donationAmount', {
currency,
- amount: amount.toFixed(2),
+ amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00',
})}📝 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.
| {tReceipt('donationDetails.donationAmount', { | |
| currency, | |
| amount: amount.toFixed(2), | |
| })} | |
| {tReceipt('donationDetails.donationAmount', { | |
| currency, | |
| amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00', | |
| })} |
| max-width: 524px; | ||
| max-height: 494px; | ||
| overflow-y: auto; | ||
| width: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 21px; | ||
| margin-top: 21px; | ||
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | ||
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential scrollbar issues on different browsers.
While you're hiding scrollbars in Firefox and IE/Edge, you're missing the WebKit implementation for Chrome and Safari.
.donationReceipts {
max-width: 524px;
max-height: 494px;
overflow-y: auto;
width: 100%;
display: flex;
flex-direction: column;
gap: 21px;
margin-top: 21px;
scrollbar-width: none; /* Hides scrollbar in Firefox */
-ms-overflow-style: none; /* Hides scrollbar in IE/Edge */
+ &::-webkit-scrollbar {
+ display: none; /* Hides scrollbar in Chrome and Safari */
+ }
}📝 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.
| max-width: 524px; | |
| max-height: 494px; | |
| overflow-y: auto; | |
| width: 100%; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 21px; | |
| margin-top: 21px; | |
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | |
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | |
| } | |
| .donationReceipts { | |
| max-width: 524px; | |
| max-height: 494px; | |
| overflow-y: auto; | |
| width: 100%; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 21px; | |
| margin-top: 21px; | |
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | |
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | |
| &::-webkit-scrollbar { | |
| display: none; /* Hides scrollbar in Chrome and Safari */ | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx (1)
72-89: 🛠️ Refactor suggestionAdd error handling to getStaticProps
The function doesn't handle potential errors when fetching tenant config or messages.
Add try/catch blocks and provide fallback values:
export const getStaticProps: GetStaticProps<PageProps> = async ( context: GetStaticPropsContext ): Promise<GetStaticPropsResult<PageProps>> => { - const tenantConfig = - (await getTenantConfig(context.params?.slug as string)) ?? defaultTenant; - - const messages = await getMessagesForPage({ - locale: context.params?.locale as string, - filenames: ['common', 'me', 'country', 'donationReceipt'], - }); + try { + const tenantConfig = + (await getTenantConfig(context.params?.slug as string)) ?? defaultTenant; + + const messages = await getMessagesForPage({ + locale: context.params?.locale as string, + filenames: ['common', 'me', 'country', 'donationReceipt'], + }); + + return { + props: { + messages, + tenantConfig, + }, + }; + } catch (error) { + console.error('Error in getStaticProps:', error); + return { + notFound: true, + }; + } - return { - props: { - messages, - tenantConfig, - }, - }; };
🧹 Nitpick comments (4)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (1)
42-75: Consider adding loading and error states.The component currently doesn't handle loading or error states. For a better user experience, consider adding indicators for when data is loading or when there's an error fetching data.
Consider adding props for loading and error states, and conditionally render appropriate UI elements such as spinners or error messages:
type Props = { currency: string; amount: number; count: number; reference: string; date: string; donations: UnissuedDonationApi[] | IssuedDonationApi[]; + isLoading?: boolean; + error?: Error | null; }; // Inside the component render: return ( <div className={styles.donationInfo}> + {isLoading && <div className={styles.loading}>Loading...</div>} + {error && <div className={styles.error}>{error.message}</div>} + {!isLoading && !error && ( <> <span className={styles.amount}> {tReceipt('donationDetails.donationAmount', { currency, amount: Number(amount).toFixed(2), })} </span> {/* Rest of the component */} </> + )} </div> );pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx (3)
44-61: Consider supporting multiple locales in getStaticPathsThe current implementation hardcodes the locale to 'en', which limits the static generation to English only. For an internationalized application, consider generating paths for all supported locales.
export const getStaticPaths: GetStaticPaths = async () => { const subDomainPaths = await constructPathsForTenantSlug(); + const supportedLocales = ['en', 'de', 'es']; // Add all supported locales const paths = subDomainPaths?.map((path) => { - return { - params: { - slug: path.params.slug, - locale: 'en', - }, - }; + return supportedLocales.map((locale) => ({ + params: { + slug: path.params.slug, + locale, + }, + })); - }) ?? []; + }).flat() ?? []; return { paths, fallback: 'blocking', }; };
30-32: Add setTenantConfig to the useEffect dependencies arrayThe effect uses
setTenantConfigbut it's not included in the dependency array. While this might work as expected sincesetTenantConfigis likely stable across renders, it's best practice to include all dependencies.useEffect(() => { if (router.isReady) setTenantConfig(tenantConfig); -}, [router.isReady]); +}, [router.isReady, setTenantConfig, tenantConfig]);
23-42: Add null check before setting tenantConfigEnsure
tenantConfigis valid before setting it to prevent potential runtime errors.useEffect(() => { - if (router.isReady) setTenantConfig(tenantConfig); + if (router.isReady && tenantConfig) setTenantConfig(tenantConfig); }, [router.isReady]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (3)
56-68: Improve accessibility for the donation count button.The button lacks proper accessibility attributes, which could make it difficult for screen reader users to understand its purpose.
Apply this diff to improve accessibility:
<button type="button" onClick={openPopover} className={styles.donationCount} + aria-label={tReceipt('aria.showDonationDetails')} + aria-haspopup="true" + aria-expanded={Boolean(popoverAnchor)} > {tReceipt.rich('itemsReferenceDateMultiDonation', { reference, count, date, u: (chunks) => <span>{chunks}</span>, })} </button>This requires adding a new translation key for the aria-label.
45-48: Add error handling for amount formatting.The
Number(amount).toFixed(2)call assumes amount is always a valid number. This could potentially cause runtime errors if amount is null, undefined, or NaN.Apply this diff to add robust error handling:
{tReceipt('donationDetails.donationAmount', { currency, - amount: Number(amount).toFixed(2), + amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00', })}
11-18: Consider adding validation for amount.The current implementation doesn't handle cases where amount might be zero or negative. Depending on your business requirements, you might want to add validation or special handling for these cases.
Consider whether zero or negative amounts are valid in your domain context and if they need special formatting or validation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (1)
42-74: Consider adding loading and error states.The component doesn't handle potential loading or error states. Consider implementing conditional rendering for these cases to improve user experience.
return ( + <> + {/* Loading state */} + {isLoading && <div className={styles.loading}>Loading donation information...</div>} + + {/* Error state */} + {error && <div className={styles.error}>{error.message}</div>} + + {/* Content when data is available */} + {!isLoading && !error && ( <div className={styles.donationInfo}> {/* ... existing content ... */} </div> + )} + </> );pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx (2)
33-50: Consider dynamic locale handling in getStaticPaths.The current implementation hard-codes the locale to 'en' for all paths. Consider generating paths for all supported locales if multiple languages are supported.
export const getStaticPaths: GetStaticPaths = async () => { const subDomainPaths = await constructPathsForTenantSlug(); + const supportedLocales = ['en', 'de', 'es']; // Add all supported locales const paths = subDomainPaths?.map((path) => { - return { - params: { - slug: path.params.slug, - locale: 'en', - }, - }; + return supportedLocales.map((locale) => ({ + params: { + slug: path.params.slug, + locale, + }, + })); - }) ?? []; + }).flat() ?? []; return { paths, fallback: 'blocking', }; };
12-19: Consider using path aliases for cleaner imports.The deep import paths (with multiple
../) make the code harder to read and maintain. Consider configuring path aliases in your tsconfig.json or Next.js config for cleaner imports.Example configuration in tsconfig.json:
{ "compilerOptions": { "baseUrl": ".", "paths": { "@features/*": ["src/features/*"], "@utils/*": ["src/utils/*"], "@config/*": ["*"] } } }Then you could update imports like:
import UserLayout from '@features/common/Layout/UserLayout/UserLayout'; import DonorContactManagement from '@features/user/DonationReceipt/DonorContactManagement'; import { constructPathsForTenantSlug, getTenantConfig, } from '@utils/multiTenancy/helpers'; import { defaultTenant } from '@config/tenant.config'; import getMessagesForPage from '@utils/language/getMessagesForPage';src/features/user/DonationReceipt/microComponents/VerifyReceiptHeader.tsx (1)
3-5: Consider organizing imports by categoryFor better readability and maintainability, consider organizing imports into groups (external libraries, internal components, types, etc.) with a blank line between groups.
import type { Operation } from '../donationReceiptTypes'; import { useTranslations } from 'next-intl'; + import styles from '../DonationReceipt.module.scss'; import { RECEIPT_STATUS } from '../donationReceiptTypes';src/features/user/DonationReceipt/microComponents/VerifyReceiptFooter.tsx (1)
8-34: Consider adding TypeScript interface for component propsEven though the component doesn't currently accept props, it's a good practice in TypeScript to define an interface for consistency and future extensibility.
+interface VerifyReceiptFooterProps {} -const VerifyReceiptFooter = () => { +const VerifyReceiptFooter: React.FC<VerifyReceiptFooterProps> = () => {src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (1)
72-72: Add error handling for invalid parametersThe component validates URL parameters but doesn't provide user feedback if they're invalid. Consider adding a case to handle and display an error message when
areParamsValidis false.if (!router.isReady) return null; + if (router.isReady && !areParamsValid) { + return ( + <ReceiptVerificationErrors + message={tReceipt.rich('errors.invalidParameters', { + b: (chunks) => <strong>{chunks}</strong>, + })} + /> + ); + } if (isReceiptInvalid)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/donationReceipt.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/VerifyReceiptFooter.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/VerifyReceiptHeader.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/user/DonationReceipt/DonationReceipt.module.scss
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (3)
47-47: Add error handling for amount formatting.The
Number(amount).toFixed(2)call could cause runtime errors if amount is not a valid number (null, undefined, or NaN).- amount: Number(amount).toFixed(2), + amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00',
56-67: Improve accessibility for the donation count button.The button has aria-haspopup and aria-expanded attributes but is missing an aria-label for screen reader users to understand its purpose.
<button type="button" onClick={openPopover} className={styles.donationCount} aria-haspopup="true" aria-expanded={Boolean(popoverAnchor)} + aria-label={tReceipt('aria.showDonationDetails')} >
63-66: Check if reference and date are needed in multi-donation translation.The translation key
itemsReferenceDateMultiDonationis used without thereferenceanddateprops that are included in the single donation case. Verify if these props should be passed to the translation.{tReceipt.rich('itemsReferenceDateMultiDonation', { count, + reference, + date, u: (chunks) => <span>{chunks}</span>, })}pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx (3)
1-20: Clean imports with proper type definitions.The file has well-structured imports with proper type definitions from Next.js, next-intl, and the application's components and utilities.
21-31: Well-implemented component with proper title.The
ModifyDonorDatacomponent correctly implements theHeadcomponent with a propertitletag, which is good for SEO and browser tab display.
57-74: Robust implementation of getStaticProps.The
getStaticPropsfunction correctly fetches tenant configuration and internationalized messages. It handles potential undefined parameters appropriately with optional chaining.src/features/user/DonationReceipt/microComponents/VerifyReceiptHeader.tsx (2)
7-25: Looks good - clean and well-structured component!This component follows React best practices with a clear single responsibility, proper TypeScript typing, and internationalization support. The conditional rendering based on the operation prop is implemented correctly.
14-23:❓ Verification inconclusive
Verify accessibility of heading structure
The component uses h2/h3 elements which is good for accessibility. However, ensure that these headings follow a proper hierarchy in the context where this component is used (i.e., they should be preceded by an h1 somewhere in the parent components).
🏁 Script executed:
#!/bin/bash # Check where this component is used and the heading structure in parent components fd "VerifyReceiptHeader" --type file --exec grep -l "VerifyReceiptHeader" {} \;Length of output: 156
Ensure Proper Heading Hierarchy in Parent Context
Your component itself correctly uses<h2>/<h3>for accessibility. However, the automated search only returned the component’s own file, so we couldn’t verify whether a parent component or page context provides a preceding<h1>. Please confirm that wheneverVerifyReceiptHeaderis used, it's embedded in a page that already includes an<h1>element to ensure a correct semantic hierarchy.src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (1)
35-70: Add missing dependencies to useEffect dependency arrayThe useEffect dependency array is still missing some dependencies that are used within the effect:
getApi,setErrors, andredirect. This could lead to stale closures if these dependencies change.}, [ dtn, year, challenge, router.isReady, areParamsValid, user, initForVerification, + getApi, + setErrors, + redirect, ]);
| <Link | ||
| href="/profile/donation-receipt" | ||
| className={styles.redirectButton} | ||
| > | ||
| <RedirectRightArrowIcon /> | ||
| </Link> | ||
| </div> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider conditional rendering of the profile link
The component shows different text for unauthenticated users, but still provides a link to the profile page that may require authentication. Consider making the link conditional or implementing a redirect to login for unauthenticated users.
🏁 Script executed:
#!/bin/bash
# Check if there's any authentication protection for the profile page
grep -r "requiresAuth" --include="*.tsx" --include="*.ts" src/
# Look for route protection related to the profile page
grep -r "/profile/donation-receipt" --include="*.tsx" --include="*.ts" src/Length of output: 721
Ensure Conditional Access for Profile Link
The donation receipt footer currently renders a link to the profile page unconditionally, even when the user is unauthenticated. Our grep search shows no evidence of an authentication guard (e.g., a requiresAuth flag) on the /profile/donation-receipt route, which means unauthenticated users might follow this link and potentially encounter issues.
- src/features/user/DonationReceipt/microComponents/VerifyReceiptFooter.tsx (Lines 22-28): The
<Link href="/profile/donation-receipt" />is rendered in all cases. - Action Required: Either conditionally render the link only when the user is authenticated or implement a mechanism (like redirecting to a login page) when an unauthenticated user attempts access.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/hooks/useServerApi.ts (6)
73-73: Simplify import path.The import path includes a redundant
../../srcprefix.-import getSessionId from '../../src/utils/apiRequests/getSessionId'; +import getSessionId from '../utils/apiRequests/getSessionId';
87-90: Consider using URL constructor for URL validation.The regex pattern works, but using the built-in URL constructor would be more robust for URL validation.
function isAbsoluteUrl(url: string) { - const pattern = /^https?:\/\//i; - return pattern.test(url); + try { + new URL(url); + return true; + } catch (e) { + return false; + } }
85-85: Use browser API for language detection.Consider using
navigator.languageas a fallback instead of hardcoding 'en'.- const lang = localStorage.getItem('language') || 'en'; + const lang = localStorage.getItem('language') || navigator.language.split('-')[0] || 'en';
121-121: Consider making the API version configurable.The API version is hardcoded to '1.0.3' when not explicitly provided. Consider making this a configurable constant or environment variable for easier updates.
- headers['x-accept-versions'] = version ? version : '1.0.3'; + const DEFAULT_API_VERSION = '1.0.3'; + headers['x-accept-versions'] = version ? version : DEFAULT_API_VERSION;
176-181: Ensure consistent parameter types across API methods.In
getApi, thepayloadparameter is used as query parameters while inpostApiit is used as the request body. Consider adding more descriptive parameter names to improve clarity.- const getApi = async <T, P extends Record<string, string> = {}>( - url: string, - payload?: P - ): Promise<T> => { - return callApi<T>({ method: 'GET', url, queryParams: payload }); + const getApi = async <T, P extends Record<string, string> = Record<string, never>>( + url: string, + queryParams?: P + ): Promise<T> => { + return callApi<T>({ method: 'GET', url, queryParams });🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
142-146: Consider more specific error handling.The catch block handles unexpected errors by logging them and throwing a generic error. Consider adding more context to the error for better debugging.
} catch (err) { if (err instanceof APIError || err instanceof ClientError) { throw err; } console.error('Unexpected error:', err); - throw new Error('An unexpected error occurred'); + throw new Error(`An unexpected error occurred while calling ${method} ${fullUrl}: ${err instanceof Error ? err.message : String(err)}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useServerApi.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/hooks/useServerApi.ts
[error] 149-149: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 163-163: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 176-176: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 183-183: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 190-190: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 197-197: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 209-209: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 218-218: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ESLint on Ubuntu
🔇 Additional comments (4)
src/hooks/useServerApi.ts (4)
1-68: Well-structured JSDoc documentation!The comprehensive documentation clearly explains the purpose, dependencies, and usage of this hook. This will greatly help other developers understand how to utilize it properly.
69-79: LGTM! Clean imports structure.The import statements are well-organized, clearly separating types and actual imports.
103-147: Good error handling in callApi function.The implementation properly handles different error types and forwards API and Client errors while wrapping unexpected errors. The token validation and URL handling are also well-implemented.
231-242: Well-structured API return object.The hook returns a well-organized set of functions that provide a clean API for consumers.
| const getApiAuthenticated = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P, | ||
| impersonationData?: ImpersonationData | ||
| ): Promise<T> => { | ||
| return callApi<T>({ | ||
| method: 'GET', | ||
| url, | ||
| queryParams: payload, | ||
| authRequired: true, | ||
| impersonationData, | ||
| }); | ||
| }; | ||
|
|
||
| const postApiAuthenticated = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P, | ||
| impersonationData?: ImpersonationData | ||
| ): Promise<T> => { | ||
| return callApi<T>({ | ||
| method: 'POST', | ||
| url, | ||
| data: payload, | ||
| authRequired: true, | ||
| impersonationData, | ||
| }); | ||
| }; | ||
| const getApi = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ method: 'GET', url, queryParams: payload }); | ||
| }; | ||
|
|
||
| const postApi = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ method: 'POST', url, data: payload }); | ||
| }; | ||
|
|
||
| const putApi = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ method: 'PUT', url, data: payload }); | ||
| }; | ||
|
|
||
| const putApiAuthenticated = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ | ||
| method: 'PUT', | ||
| url, | ||
| data: payload, | ||
| authRequired: true, | ||
| }); | ||
| }; | ||
|
|
||
| const deleteApi = async <T, P extends Record<string, string> = {}>( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ method: 'DELETE', url, queryParams: payload }); | ||
| }; | ||
|
|
||
| const deleteApiAuthenticated = async < | ||
| T, | ||
| P extends Record<string, string> = {} | ||
| >( | ||
| url: string, | ||
| payload?: P | ||
| ): Promise<T> => { | ||
| return callApi<T>({ | ||
| method: 'DELETE', | ||
| url, | ||
| queryParams: payload, | ||
| authRequired: true, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type constraint issue in API methods.
The static analysis tools correctly identified an issue with using {} as a type. This can lead to unexpected behavior because {} allows any non-null value, not just objects.
Fix the type constraint across all API methods with a more specific type:
- const getApiAuthenticated = async <T, P extends Record<string, string> = {}>(
+ const getApiAuthenticated = async <T, P extends Record<string, string> = Record<string, never>>(Apply this change to all similar function signatures in the file (lines 149, 163, 176, 183, 190, 197, 209, and 216).
📝 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 getApiAuthenticated = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P, | |
| impersonationData?: ImpersonationData | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'GET', | |
| url, | |
| queryParams: payload, | |
| authRequired: true, | |
| impersonationData, | |
| }); | |
| }; | |
| const postApiAuthenticated = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P, | |
| impersonationData?: ImpersonationData | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'POST', | |
| url, | |
| data: payload, | |
| authRequired: true, | |
| impersonationData, | |
| }); | |
| }; | |
| const getApi = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'GET', url, queryParams: payload }); | |
| }; | |
| const postApi = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'POST', url, data: payload }); | |
| }; | |
| const putApi = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'PUT', url, data: payload }); | |
| }; | |
| const putApiAuthenticated = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'PUT', | |
| url, | |
| data: payload, | |
| authRequired: true, | |
| }); | |
| }; | |
| const deleteApi = async <T, P extends Record<string, string> = {}>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'DELETE', url, queryParams: payload }); | |
| }; | |
| const deleteApiAuthenticated = async < | |
| T, | |
| P extends Record<string, string> = {} | |
| >( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'DELETE', | |
| url, | |
| queryParams: payload, | |
| authRequired: true, | |
| }); | |
| }; | |
| const getApiAuthenticated = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P, | |
| impersonationData?: ImpersonationData | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'GET', | |
| url, | |
| queryParams: payload, | |
| authRequired: true, | |
| impersonationData, | |
| }); | |
| }; | |
| const postApiAuthenticated = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P, | |
| impersonationData?: ImpersonationData | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'POST', | |
| url, | |
| data: payload, | |
| authRequired: true, | |
| impersonationData, | |
| }); | |
| }; | |
| const getApi = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'GET', url, queryParams: payload }); | |
| }; | |
| const postApi = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'POST', url, data: payload }); | |
| }; | |
| const putApi = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'PUT', url, data: payload }); | |
| }; | |
| const putApiAuthenticated = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'PUT', | |
| url, | |
| data: payload, | |
| authRequired: true, | |
| }); | |
| }; | |
| const deleteApi = async <T, P extends Record<string, string> = Record<string, never>>( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ method: 'DELETE', url, queryParams: payload }); | |
| }; | |
| const deleteApiAuthenticated = async < | |
| T, | |
| P extends Record<string, string> = Record<string, never> | |
| >( | |
| url: string, | |
| payload?: P | |
| ): Promise<T> => { | |
| return callApi<T>({ | |
| method: 'DELETE', | |
| url, | |
| queryParams: payload, | |
| authRequired: true, | |
| }); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 163-163: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 176-176: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 183-183: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 190-190: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 197-197: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 209-209: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 218-218: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (1)
35-62: 🛠️ Refactor suggestionAdd missing dependencies to useEffect dependency array.
The useEffect dependency array is missing several dependencies that are used within the effect:
getApi,initForVerification,setErrors, andredirect. This could lead to stale closures if these dependencies change.- }, [dtn, year, challenge, router.isReady, areParamsValid, user]); + }, [dtn, year, challenge, router.isReady, areParamsValid, user, getApi, initForVerification, setErrors, redirect];If these functions are stable (e.g., created with useCallback or useMemo), you can safely ignore this suggestion, but it would be a good practice to add a comment explaining why they are intentionally omitted.
🧹 Nitpick comments (3)
src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (3)
35-62: Consider adding a cleanup function to the useEffect.If the component unmounts during the API call, state updates might still occur on an unmounted component. Add a cleanup function to prevent this.
useEffect(() => { if (!router.isReady || !areParamsValid) return; + let isComponentMounted = true; (async () => { setIsLoading(true); try { const url = `/app/donationReceipt?dtn=${encodeURIComponent( dtn )}&year=${encodeURIComponent(year)}&challenge=${encodeURIComponent( challenge )}`; const data = await getApi<IssuedReceiptDataApi>(url); - if (data) initForVerification(data, user); + if (data && isComponentMounted) initForVerification(data, user); } catch (err) { const errorResponse = err as APIError; + if (!isComponentMounted) return; setErrors(handleError(errorResponse)); if (errorResponse.statusCode === 400) { setIsReceiptInvalid(true); } else { redirect('/'); } } finally { + if (isComponentMounted) { setIsLoading(false); + } } })(); + return () => { + isComponentMounted = false; + }; }, [dtn, year, challenge, router.isReady, areParamsValid, user, getApi, initForVerification, setErrors, redirect]);
51-57: Enhance error handling with more specific error messages.The current error handling is basic and only checks for status code 400. Consider adding more specific error messages for different error scenarios to improve user experience.
} catch (err) { const errorResponse = err as APIError; setErrors(handleError(errorResponse)); - if (errorResponse.statusCode === 400) { + if (errorResponse.statusCode === 400) { setIsReceiptInvalid(true); + } else if (errorResponse.statusCode === 401 || errorResponse.statusCode === 403) { + // Handle authentication/authorization errors + setErrors([{ message: tReceipt('errors.unauthorized') }]); + redirect('/login'); + } else if (errorResponse.statusCode === 404) { + // Handle not found errors + setErrors([{ message: tReceipt('errors.receiptNotFound') }]); + setIsReceiptInvalid(true); } else { redirect('/'); }
30-34: Consider enhancing parameter validation.The current validation only checks if parameters are strings but doesn't validate their format or content. Consider adding more robust validation to prevent potential issues.
const areParamsValid = typeof dtn === 'string' && typeof year === 'string' && typeof challenge === 'string'; + + // Additional validation for parameter format + const isValidYear = typeof year === 'string' && /^\d{4}$/.test(year); + const isValidDtn = typeof dtn === 'string' && dtn.trim().length > 0; + const isValidChallenge = typeof challenge === 'string' && challenge.trim().length > 0; + + const areParamsFormatValid = isValidYear && isValidDtn && isValidChallenge;Then update the condition in the useEffect:
- if (!router.isReady || !areParamsValid) return; + if (!router.isReady || !areParamsValid || !areParamsFormatValid) { + if (router.isReady && !areParamsFormatValid) { + setIsReceiptInvalid(true); + } + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (3)
1-17: LGTM: Imports are well-organized.The imports are well-organized with type imports at the top, followed by functional imports grouped by category.
18-29: LGTM: Component setup and hook initialization.The component setup and hook initialization follow best practices. The component correctly uses necessary hooks for routing, translations, API calls, state management, and context access.
64-83: LGTM: Well-structured conditional rendering.The conditional rendering logic is clear and follows a good pattern for handling different component states (loading, error, ready). The loading skeleton provides a good user experience while data is being fetched.
Introduce a new DonationReceipt page with components for displaying donation details, recipient information, and actions. Enhance user experience with improved validation, error handling, and mobile responsiveness. Refactor existing code for better structure and maintainability.