Fix/cashout bugs6 #521
Fix/cashout bugs6 #521
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request includes significant changes across multiple files, primarily focusing on enhancing API response handling, error management, and user interface components. Key modifications involve updating the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
2 similar comments
| @@ -1,4 +1,4 @@ | |||
| import * as components from '@/components' | |||
| import { Cashout } from '@/components' | |||
| }, []) | ||
|
|
||
| // For testing - check if URL has screen param | ||
| useEffect(() => { |
There was a problem hiding this comment.
Should this stil be in here if it was for testing ?
There was a problem hiding this comment.
yes! Its useful
peanut.to/cashout?screen=confirm
No reason not to, way easier for future testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (33)
src/app/cashout/page.tsx (1)
Line range hint
6-6: Add page metadata for SEO optimization.Consider implementing Next.js metadata for better SEO and social sharing capabilities.
Would you like me to help generate the metadata configuration? Here's an example of what could be added:
export const metadata = { title: 'Cashout - Peanut Protocol', description: 'Securely cash out your crypto assets with Peanut Protocol', }src/components/Cashout/Components/RecipientInfo.comp.tsx (1)
1-11: Consider future-proofing the recipient info display.The PR objectives mention a limitation regarding suggesting both IBAN and US accounts simultaneously. Consider implementing the tooltip content and UI structure in a way that would make it easier to support multiple account types in the future.
Some suggestions:
- Structure tooltip content to be dynamically generated based on available account types
- Consider a design pattern that would allow easy addition of new account types
- Plan for a future migration path to support multiple account suggestions
src/constants/tooltips.ts (3)
2-10: Consider improving accessibility and maintainability of RECIPIENT_INFO tooltipThe current implementation has several areas for improvement:
- HTML markup in string templates can be problematic for accessibility and maintenance
- Instructions for different account types (IBAN vs US) should be clearly separated
- External link handling could be enhanced for security
Consider refactoring to:
RECIPIENT_INFO: `You can claim directly to your IBAN OR US bank account. -For US bank accounts, enter your bank routing number (9 digits) and account number together. +For US bank accounts: +• Enter your bank routing number (9 digits) and account number together +• Example: 1112223330001234567 + - Routing number (9 digits): 111222333 + - Account number: 0001234567 -Example US bank account: 1112223330001234567, with: -Routing number (9 digits): 111222333 -Account number: 0001234567 -<a href="https://docs.peanut.to/app/cashout/supported-geographies" target="_blank" class="underline text-blue-600">Supported regions</a>`, +[Learn more about supported regions →](https://docs.peanut.to/app/cashout/supported-geographies)`,
12-18: Enhance CLAIM_RECIPIENT_INFO clarity and completenessThe tooltip could benefit from additional information and consistent formatting:
- Consider adding important limitations or restrictions
- Bullet point formatting is inconsistent
Consider updating to:
CLAIM_RECIPIENT_INFO: `You can claim your funds to: -• Any Ethereum wallet address -• ENS domain (.eth) -• EU bank account (IBAN) -• US bank account (routing + account number) +• Crypto: + - Any Ethereum wallet address + - ENS domain (.eth) +• Bank accounts: + - EU bank account (IBAN) + - US bank account (routing + account number) -<a href="https://docs.peanut.to/app/cashout/supported-geographies" target="_blank" class="underline text-blue-600">Learn more about supported regions</a>`, +Note: Minimum and maximum amounts may apply. +[Learn more about supported regions →](https://docs.peanut.to/app/cashout/supported-geographies)`,
1-29: Consider implementing a more maintainable tooltip systemThe current implementation could benefit from several architectural improvements:
- Content Management: Consider moving tooltip content to a CMS or separate content files for easier maintenance
- Internationalization: Implement i18n support for multi-language capabilities
- Component-based approach: Create a dedicated tooltip component system that handles formatting and styling consistently
This would make the system more maintainable and scalable for future enhancements.
src/app/api/bridge/external-account/get-all-for-customerId/route.ts (1)
23-28: Consider adding type safety and improving error responses.While the current implementation works, we could enhance it further:
- Add TypeScript interfaces for the Bridge API response
- Provide more structured error responses with error codes
Consider applying these improvements:
+interface BridgeAccount { + id: string; + // add other account properties +} + +interface BridgeApiResponse { + data: BridgeAccount[]; +} + export async function POST(request: NextRequest) { try { const { customerId } = await request.json() // ... - const responseData = await response.json() + const responseData = (await response.json()) as BridgeApiResponse // Bridge API returns { data: Account[] }, we want to return just the array const accounts = responseData.data || [] return new NextResponse(JSON.stringify(accounts), { status: 200, headers: { 'Content-Type': 'application/json', }, }) } catch (error) { - console.error('Failed to fetch external accounts:', error) - return new NextResponse('Internal Server Error', { status: 500 }) + console.error('Failed to fetch external accounts:', error instanceof Error ? error.message : error) + return new NextResponse( + JSON.stringify({ + error: 'Failed to fetch external accounts', + details: error instanceof Error ? error.message : 'Unknown error' + }), + { + status: 500, + headers: { 'Content-Type': 'application/json' } + } + ) } }src/components/Global/AddressLink/index.tsx (1)
9-15: Consider making the ENS lookup chain configurable.While using mainnet (chain 1) for ENS lookups is currently correct, hardcoding the chainId might make the component less flexible if the app needs to support alternative networks with their own naming services in the future.
Consider this improvement:
+interface AddressLinkProps { + address: string; + ensChainId?: number; +} -const AddressLink = ({ address }: { address: string }) => { +const AddressLink = ({ address, ensChainId = 1 }: AddressLinkProps) => { // ... const { data: ensName } = useEnsName({ address: isAddress(address) ? (address as `0x${string}`) : undefined, - chainId: 1, // Mainnet for ENS lookups + chainId: ensChainId, }) // ...src/components/Global/RecipientInput/index.tsx (1)
28-40: Consider adding tooltip for clear button.Since this is part of the cashout flow where tooltips are being improved (per PR objectives), consider adding a tooltip to the clear button for better user guidance.
<button onClick={(e) => { e.preventDefault() setValue('') }} className="flex h-full w-12 items-center justify-center bg-white focus:outline-none focus:ring-2 focus:ring-primary" aria-label="Clear input" + title="Clear input" >src/hooks/useRecentRecipients.ts (3)
3-4: Consider using a more specific storage key.The current storage key
'recent-recipients'could potentially conflict with other applications using localStorage. Consider prefixing it with your application name, e.g.,'peanut-recent-recipients'.-const STORAGE_KEY = 'recent-recipients' +const STORAGE_KEY = 'peanut-recent-recipients'
38-43: Consider adding sorting options to getSuggestions.The timestamp field isn't being utilized for sorting. Consider adding sorting options to getSuggestions to make better use of the timestamp.
-const getSuggestions = (type?: RecentRecipient['type']) => { +const getSuggestions = (type?: RecentRecipient['type'], sort: 'recent' | 'alpha' = 'recent') => { if (type) { - return recentRecipients.filter((r) => r.type === type).map((r) => r.value) + const filtered = recentRecipients.filter((r) => r.type === type) + return sortRecipients(filtered, sort).map((r) => r.value) } - return recentRecipients.map((r) => r.value) + return sortRecipients(recentRecipients, sort).map((r) => r.value) } + +const sortRecipients = (recipients: RecentRecipient[], sort: 'recent' | 'alpha') => { + return [...recipients].sort((a, b) => { + if (sort === 'recent') { + return b.timestamp - a.timestamp + } + return a.value.localeCompare(b.value) + }) +}
45-50: Consider exposing the clear function in the hook's return value.If you implement the suggested clearRecipients function, it should be exposed in the hook's return value.
return { recentRecipients, addRecipient, getSuggestions, + clearRecipients, }src/components/Global/CountrySelect/index.tsx (2)
12-13: Consider expanding priority countries for better UX.Given that the cashout flow handles both US and IBAN accounts, consider including major EU countries in the priority list to improve user experience for both US and EU users.
-const PRIORITY_COUNTRIES = ['USA', 'CAN'] +const PRIORITY_COUNTRIES = ['USA', 'CAN', 'GBR', 'DEU', 'FRA']
44-45: Consider using Tailwind's dark variant consistently.The dark mode styling could be more consistent by using Tailwind's dark variant for all dark mode-specific styles.
-classButton={`h-12 w-full bg-white px-4 py-2 font-normal dark:bg-n-1 - ${error ? 'border-red' : 'border border-n-1 dark:border-white'}`} +classButton={`h-12 w-full bg-white dark:bg-n-1 px-4 py-2 font-normal + ${error ? 'border-red' : 'border border-n-1 dark:border-white'}`}src/app/api/bridge/liquidation-address/create/route.ts (1)
12-12: Consider using a more generic error message.While the error handling improvement is good, exposing the exact missing environment variable in the error message might reveal internal implementation details.
- return NextResponse.json({ error: 'BRIDGE_API_KEY is not defined' }, { status: 500 }) + return NextResponse.json({ error: 'Internal configuration error' }, { status: 500 })src/components/Global/GeneralRecipientInput/index.tsx (3)
26-32: Consider enhancing type safety with required propsWhile the implementation is correct, consider marking essential props as required by removing the optional modifier from their types in
GeneralRecipientInputProps.type GeneralRecipientInputProps = { className?: string - placeholder: string - recipient: { name: string | undefined; address: string } - onUpdate: (update: GeneralRecipientUpdate) => void + placeholder: string // Required + recipient: { name: string | undefined; address: string } // Required + onUpdate: (update: GeneralRecipientUpdate) => void // Required infoText?: string }
61-61: Consider moving error state to useStateUsing a ref for error messages might cause issues with React's rendering lifecycle. Consider using useState instead.
+const [errorMessage, setErrorMessage] = useState(''); -const errorMessage = useRef(''); // Then update the error messages using setErrorMessage -errorMessage.current = 'ENS name not found'; +setErrorMessage('ENS name not found');Also applies to: 67-67
77-106: Simplify update object constructionThe update object construction is verbose and could be simplified using object spread.
- const onInputUpdate = useCallback( - (update: InputUpdate) => { - let _update: GeneralRecipientUpdate - if (update.isValid) { - errorMessage.current = '' - _update = { - recipient: - 'ens' === recipientType.current - ? { address: resolvedAddress.current, name: update.value } - : { address: update.value, name: undefined }, - type: recipientType.current, - isValid: true, - isChanging: update.isChanging, - errorMessage: '', - } - addRecipient(update.value, recipientType.current) + const onInputUpdate = useCallback((update: InputUpdate) => { + const baseUpdate = { + type: recipientType.current, + isChanging: update.isChanging, + isValid: update.isValid, + }; + + if (update.isValid) { + const recipient = recipientType.current === 'ens' + ? { address: resolvedAddress.current, name: update.value } + : { address: update.value, name: undefined }; + + addRecipient(update.value, recipientType.current); + onUpdate({ + ...baseUpdate, + recipient, + errorMessage: '', + });src/components/Global/ValidatedInput/index.tsx (1)
39-39: Consider using a more stable ID generation method.Using
Math.random()for ID generation could cause hydration mismatches in SSR. Consider using a deterministic ID based on props or a stable unique ID generator.-const listId = useRef(`datalist-${Math.random().toString(36).substr(2, 9)}`) +const listId = useRef(`datalist-${label.toLowerCase().replace(/\s+/g, '-')}`)src/components/Cashout/Cashout.tsx (3)
Line range hint
7-7: Add proper TypeScript interface for component props.The component accepts empty props but uses
offrampTypein rendering. This could lead to runtime errors. Consider adding proper prop typing:+ interface CashoutProps { + offrampType: OfframpType; + } - export const Cashout = ({}) => { + export const Cashout = ({ offrampType }: CashoutProps) => {
Line range hint
8-52: Optimize state management for better maintainability.The component has numerous state variables that could be better organized. Consider:
- Using
useReducerfor related state (e.g., form state, transaction state)- Combining related state into objects
- Using derived state where possible
Example refactor for form-related state:
interface FormState { offramp: { name: string; email: string; password: string; recipient: string; }; values: { token?: string; usd?: string; }; recipient: { address?: string; name?: string; }; } const formReducer = (state: FormState, action: FormAction) => { switch (action.type) { case 'UPDATE_OFFRAMP': return { ...state, offramp: { ...state.offramp, ...action.payload } }; // ... other cases } }; // In component: const [formState, dispatchForm] = useReducer(formReducer, initialFormState);
Line range hint
66-77: Improve error handling and add loading states.The API call handling could be improved with:
- Proper loading states
- Error state management
- Cleanup for unmounted components
+ const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<Error | null>(null); const fetchAndSetCrossChainDetails = async () => { + setIsLoading(true); + setError(null); try { const response = await fetch('https://apiplus.squidrouter.com/v2/chains', { headers: { 'x-integrator-id': '11CBA45B-5EE9-4331-B146-48CCD7ED4C7C', }, }); if (!response.ok) { throw new Error('Squid: Network response was not ok'); } const data = await response.json(); setCrossChainDetails(data.chains); + } catch (err) { + setError(err instanceof Error ? err : new Error('Unknown error')); + } finally { + setIsLoading(false); } } useEffect(() => { + let mounted = true; fetchAndSetCrossChainDetails(); + return () => { + mounted = false; + }; }, []);src/components/Offramp/Success.view.tsx (1)
Line range hint
59-94: Optimize account lookup and improve error handling.The current implementation has several areas for improvement:
- Repeated account lookup logic
- Multiple string manipulations
- No error handling for invalid USD values
- Duplicated fee information in tooltips
Consider refactoring to improve maintainability and reliability:
+ // At the top of the component + const selectedAccount = user?.accounts.find( + (account) => account?.account_identifier?.replaceAll(/\s/g, '').toLowerCase() === + offrampForm.recipient?.replaceAll(/\s/g, '').toLowerCase() + ); + + const calculateAmountReceived = (amount: string, accountType?: string) => { + if (!amount || isNaN(parseFloat(amount))) return 'Invalid amount'; + const fee = accountType === 'iban' ? 1 : 0.5; + return utils.formatTokenAmount(parseFloat(amount) - fee); + }; // In the render section - {user?.accounts.find( - (account) => - account.account_identifier.replaceAll(/\s/g, '').toLowerCase() === - offrampForm.recipient.replaceAll(/\s/g, '').toLowerCase() - )?.account_type === 'iban' - ? utils.formatTokenAmount(parseFloat(usdValue ?? '') - 1) - : utils.formatTokenAmount(parseFloat(usdValue ?? '') - 0.5)} + ${calculateAmountReceived(usdValue ?? '', selectedAccount?.account_type)} - text={ - user?.accounts.find( - (account) => - account.account_identifier.replaceAll(/\s/g, '').toLowerCase() === - offrampForm.recipient.replaceAll(/\s/g, '').toLowerCase() - )?.account_type === 'iban' - ? 'For SEPA transactions...' - : 'For ACH transactions...' - } + text={`Transaction fees will be deducted from the amount you receive. ${ + TRANSACTION_FEES[selectedAccount?.account_type ?? '']?.label ?? 'Unknown' + } transactions incur a fee of $${ + TRANSACTION_FEES[selectedAccount?.account_type ?? '']?.amount ?? 'Unknown' + }.`}src/components/Cashout/Components/Initial.view.tsx (1)
Line range hint
337-359: Fix error message formattingThe error message contains a line continuation character (
\) that might cause display issues. Consider using template literals for multi-line strings.Apply this diff:
- 'Invalid bank account. For US bank accounts, enter your bank routing number (9 digits) followed by your account number \ - (example: 1112223330001234567 where routing number is: 111222333 and account number: 0001234567)', + `Invalid bank account. For US bank accounts, enter your bank routing number (9 digits) followed by your account number + (example: 1112223330001234567 where routing number is: 111222333 and account number: 0001234567)`,The rest of the changes look good:
- Proper form attributes for browser autofill
- Consistent case handling (lowercase storage, uppercase display)
src/components/Global/LinkAccountComponent/index.tsx (2)
245-245: Consider additional autocomplete enhancements.While the current autocomplete attributes are good, consider adding these additional attributes to improve the user experience:
<input {...registerIban('accountNumber', { required: 'This field is required', })} className={`custom-input ${ibanErrors.accountNumber ? 'border border-red' : ''}`} placeholder={'Bank account'} autoComplete="on" name="bank-account" + inputMode="numeric" + pattern="[0-9]*" />For address fields:
<input {...registerAccountDetails('street', { required: getAccountDetailsValue('type') === 'us' ? 'This field is required' : false, })} className={`custom-input ${accountDetailsErrors.street ? 'border border-red' : ''}`} placeholder="Your street and number" autoComplete="street-address" name="street" + autoComplete="address-line1" />Also applies to: 316-317, 336-337, 355-356, 376-377
Line range hint
1-479: Consider additional security measures for handling sensitive banking data.While the current implementation handles input sanitization well, consider these security enhancements:
- Add rate limiting for account linking attempts
- Implement input masking for sensitive fields
- Add client-side validation for routing numbers using a checksum algorithm
- Consider implementing secure form field features:
// Add these props to sensitive input fields const securityProps = { autoComplete: "off", autoCorrect: "off", spellCheck: false, autoCapitalize: "off", // Prevent password managers from capturing bank details data-lpignore: "true" };src/components/Claim/Link/Initial.view.tsx (2)
Line range hint
473-486: Consider simplifying the placeholder text.The current placeholder text is quite long and combines multiple input types. Consider breaking it down or making it more concise for better readability.
- placeholder="wallet address / ENS / IBAN / US account number" + placeholder="Enter wallet address, ENS, or bank details"
646-664: Enhance error message presentation and formatting.While the error handling logic is good, consider these improvements:
- Use template literals to interpolate the MIN_CASHOUT_LIMIT and MAX_CASHOUT_LIMIT values
- Make error messages more user-friendly
- You can not claim links with less than ${MIN_CASHOUT_LIMIT} to your bank - account. + {`The minimum amount for bank transfers is $${MIN_CASHOUT_LIMIT}`} - You can not claim links with more than ${MAX_CASHOUT_LIMIT} to your bank - account. + {`The maximum amount for bank transfers is $${MAX_CASHOUT_LIMIT}`}src/components/Offramp/Confirm.view.tsx (1)
668-685: Consider refactoring the route display section for better maintainability.The current implementation could be improved in several ways:
- Extract the chain lookup logic into a separate function
- Improve the tooltip text to be more informative
- Simplify the nested structure
Consider this refactor:
+ const getChainName = (chainId: string) => + consts.supportedPeanutChains.find(chain => chain.chainId === chainId)?.name <div className="flex w-full flex-row items-center px-2 text-h8 text-gray-1"> <div className="flex w-1/3 flex-row items-center gap-1"> <Icon name={'forward'} className="h-4 fill-gray-1" /> <label className="font-bold">Route</label> </div> <div className="relative flex flex-1 items-center justify-end gap-1 text-sm font-normal"> <div className="flex items-center gap-1"> - {consts.supportedPeanutChains.find( - (chain) => chain.chainId === claimLinkData?.chainId - )?.name}{' '} + {getChainName(claimLinkData?.chainId)}{' '} <Icon name={'arrow-next'} className="h-4 fill-gray-1" /> Offramp{' '} <Icon name={'arrow-next'} className="h-4 fill-gray-1" />{' '} {recipientType?.toUpperCase()}{' '} - <MoreInfo text={`Wait, crypto can be converted to real money??? How cool!`} /> + <MoreInfo text={`This shows the route your crypto will take: from the source chain through our offramp service to your bank account.`} /> </div> </div> </div>src/components/Global/MoreInfo/index.tsx (1)
14-19: Simplify code by removing unnecessarymountedstateThe
mountedstate variable may be unnecessary for controlling the rendering of the tooltip. Since React handles component unmounting gracefully, you can simplify the code by removing themountedstate and its associated logic.Apply this diff to remove the
mountedstate:- const [mounted, setMounted] = useState(false) ... - useEffect(() => { - setMounted(true) - return () => setMounted(false) - }, []) ... - if (!mounted) return nullAlso applies to: 67-67
src/components/Global/IframeWrapper/index.tsx (2)
94-97: Provide user feedback when KYC status polling failsWhen an error occurs during the KYC status polling, the user is not notified. Consider displaying a toast notification to inform the user about the error, so they are aware that the KYC status could not be retrieved.
Apply this diff to display an error toast:
} catch (error) { console.error('❌ Error polling KYC status:', error) setIsPolling(false) + toast({ + title: 'Error', + description: 'Failed to retrieve KYC status. Please try again later.', + status: 'error', + duration: 5000, + isClosable: true, + }) }
126-129: Improve robustness ofsrccheck in modal close logicUsing
src.includes('tos')to determine whether to bypass the close confirmation might be unreliable if thesrcstring containstosunexpectedly. Consider introducing an explicit flag or prop to indicate when the confirmation prompt should be skipped, enhancing the reliability of this check.src/components/Global/KYCComponent/index.tsx (1)
166-173: Ensure Complete Reset of Iframe Options Before KYCWhile resetting iframe options before moving to KYC, consider resetting all properties to their default values to prevent unintended carry-over of previous states.
src/utils/cashout.utils.ts (1)
303-405: Maintain Consistent Function Declaration StyleThere is an inconsistency in function declaration styles within the file. Some functions use
export async functionwhile others useexport const functionName = async () => {}. For consistency and readability, consider adhering to a single style throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
src/app/api/bridge/external-account/get-all-for-customerId/route.ts(1 hunks)src/app/api/bridge/liquidation-address/create/route.ts(3 hunks)src/app/cashout/page.tsx(2 hunks)src/app/layout.tsx(2 hunks)src/app/request/create/page.tsx(2 hunks)src/components/Cashout/Cashout.tsx(2 hunks)src/components/Cashout/Components/Faq.comp.tsx(1 hunks)src/components/Cashout/Components/Initial.view.tsx(7 hunks)src/components/Cashout/Components/RecipientInfo.comp.tsx(1 hunks)src/components/Claim/Link/Initial.view.tsx(8 hunks)src/components/Global/AddressLink/index.tsx(1 hunks)src/components/Global/Banner/index.tsx(1 hunks)src/components/Global/CountrySelect/index.tsx(1 hunks)src/components/Global/GeneralRecipientInput/index.tsx(4 hunks)src/components/Global/IframeWrapper/index.tsx(1 hunks)src/components/Global/KYCComponent/index.tsx(13 hunks)src/components/Global/LinkAccountComponent/index.tsx(8 hunks)src/components/Global/MoreInfo/index.tsx(1 hunks)src/components/Global/RecipientInput/index.tsx(1 hunks)src/components/Global/ValidatedInput/index.tsx(4 hunks)src/components/Offramp/Confirm.view.tsx(1 hunks)src/components/Offramp/Success.view.tsx(2 hunks)src/constants/tooltips.ts(1 hunks)src/hooks/useRecentRecipients.ts(1 hunks)src/utils/cashout.utils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Global/Banner/index.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Cashout/Components/Initial.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#484
File: src/components/Cashout/Components/Initial.view.tsx:273-274
Timestamp: 2024-10-25T11:33:46.776Z
Learning: In the `InitialCashoutView` component (`src/components/Cashout/Components/Initial.view.tsx`), linked bank accounts should not generate error states, and the `ValidatedInput` component will clear any error messages if needed. Therefore, it's unnecessary to manually clear the error state when selecting or clearing linked bank accounts.
🪛 Biome
src/components/Cashout/Components/Initial.view.tsx
[error] 82-82: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 88-88: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
src/components/Global/MoreInfo/index.tsx
[error] 84-84: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (45)
src/app/cashout/page.tsx (2)
1-1: LGTM! Import and component usage improvements.
The change from wildcard import to specific import is a good practice that improves code maintainability and potentially reduces bundle size.
Also applies to: 11-11
3-3: Verify the necessity of force-dynamic rendering.
The force-dynamic setting ensures the page is always rendered at request time. While this might be necessary for real-time cashout functionality, it could impact performance.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the findings provide clear evidence about the dynamic nature of the component.
The force-dynamic setting is justified for this page
The Cashout component heavily relies on dynamic data and state management:
- Makes API calls to fetch cross-chain details from
apiplus.squidrouter.com - Uses multiple
useEffecthooks for initialization and URL parameter handling - Manages extensive client-side state with numerous
useStatehooks for:- Transaction details (hash, link, type)
- Form data (offramp details, recipient info)
- Cross-chain data
- KYC steps
- Fee calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dynamic data requirements in the Cashout component
# Look for API calls, hooks, or other dynamic data fetching
# Search for data fetching patterns in the Cashout component
rg -A 5 "useEffect|fetch|axios|api" src/components/Cashout/Cashout.tsx
# Look for state management that might require dynamic rendering
rg -A 5 "useState|useReducer|useContext" src/components/Cashout/Cashout.tsx
Length of output: 3765
src/components/Cashout/Components/Faq.comp.tsx (1)
1-2: LGTM! Clean import structure.
The imports are well-organized, bringing in the necessary components and constants.
src/components/Cashout/Components/RecipientInfo.comp.tsx (1)
8-8: Verify tooltip content completeness.
The implementation is clean and follows good practices. However, let's ensure the tooltip content provides sufficient guidance for both IBAN and US bank account users.
✅ Verification successful
Tooltip content is complete and well-documented
The TOOLTIPS.RECIPIENT_INFO content provides comprehensive guidance for both IBAN and US bank account users, including:
- Clear options for both IBAN and US bank accounts
- Detailed example for US bank account format (routing + account numbers)
- Link to supported regions documentation
- Proper formatting with HTML support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the content of RECIPIENT_INFO tooltip
# Test: Check tooltip content in constants file
rg -A 10 "RECIPIENT_INFO.*=" src/constants/tooltips.ts
Length of output: 56
Script:
#!/bin/bash
# Let's try different approaches to locate the tooltips
# Search for TOOLTIPS or tooltip-related files
fd -t f -e ts -e tsx tooltip
# Search for any constant containing RECIPIENT
rg -i "recipient.*=.*" --type ts --type tsx
# Search for files that might contain tooltip definitions
fd -t f constants
Length of output: 150
Script:
#!/bin/bash
# Now that we found the tooltips file, let's examine its content
cat src/constants/tooltips.ts
# Also search for any RECIPIENT related strings without file type restriction
rg -i "recipient.*="
Length of output: 9137
src/app/request/create/page.tsx (2)
3-3: LGTM! Improved import specificity.
The change from wildcard import to specific component import is a good practice that enables better tree-shaking and potentially reduces bundle size.
23-23: LGTM! Simplified component usage.
Direct component usage instead of dot notation improves code clarity and maintains consistency with the specific import approach.
src/constants/tooltips.ts (1)
20-28: 🛠️ Refactor suggestion
Consider extracting fee information and adding disclaimers
The FAQ contains hardcoded fee values and processing times that might need frequent updates. Additionally, important disclaimers about bank fees are missing.
Consider:
- Moving fee values to a separate configuration file
- Adding disclaimers about additional bank fees
- Clarifying processing time variations
Suggested implementation:
CASHOUT_FAQ: `• What currencies can I cash out?
Most popular tokens and stablecoins are supported.
• How long does it take?
- Usually 1-2 business days.
+ • SEPA (EU): 1-2 business days typically
+ • ACH (US): 2-3 business days typically
+ Note: Processing times may vary based on your bank.
• What are the fees?
- SEPA (EU): $1
- ACH (US): $0.50`,
+ ${CASHOUT_FEES.SEPA_EU}
+ ${CASHOUT_FEES.ACH_US}
+ Note: Your bank may charge additional fees.`,src/app/api/bridge/external-account/get-all-for-customerId/route.ts (2)
23-28: LGTM! Clean response data transformation.
The changes improve the API response structure by returning only the necessary accounts array, making it cleaner for the client to consume.
23-28: Verify error handling in the client code.
Since we're changing the response structure, we should ensure that client-side code properly handles both success and error cases.
Let's check the client-side usage:
✅ Verification successful
Error handling is properly implemented in client code
The client code shows proper error handling for the API endpoint:
- The main usage in
getExternalAccountsfunction usesfetchApiutility which has built-in error handling that throws ifresponse.okis false. - In
createExternalAccountfunction, there's comprehensive error handling:- Handles duplicate account cases by fetching all accounts and finding the matching one
- Proper error propagation with detailed error messages
- JSON parsing errors are caught and handled
- Network errors are caught and logged
The response structure change (removing the data wrapper) is safely handled as the client code properly processes the response data in all cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where this API endpoint is called
rg -l "get-all-for-customerId" --type ts --type tsx
# Look for error handling patterns in those files
rg "catch.*get-all-for-customerId" --type ts --type tsx -A 5
Length of output: 170
Script:
#!/bin/bash
# Search for places where this API endpoint is called, using ripgrep without file type filters
rg -l "get-all-for-customerId"
# Look for error handling patterns in those files
rg "catch.*get-all-for-customerId" -A 5
Length of output: 99
Script:
#!/bin/bash
# Let's examine the usage in cashout.utils.ts
cat src/utils/cashout.utils.ts
# Also search for any fetch or API calls to this endpoint
rg -A 5 -B 5 "/api/bridge/external-account/get-all-for-customerId"
Length of output: 25136
src/components/Global/AddressLink/index.tsx (2)
3-4: LGTM! Good choice of dependencies.
The addition of useEnsName and isAddress from established Ethereum libraries provides robust ENS resolution and address validation capabilities.
17-27: LGTM! Clean and efficient effect implementation.
The effect properly handles both URL and display updates with appropriate dependencies. While it could be split into separate effects for URL and display updates, the current implementation is clean and maintainable given their shared dependency on the address.
src/app/layout.tsx (1)
43-50: LGTM!
The component structure follows best practices with proper provider hierarchy and consistent indentation.
src/hooks/useRecentRecipients.ts (1)
6-10: LGTM! Well-structured interface definition.
The interface is well-defined with appropriate types and follows TypeScript best practices.
src/components/Global/CountrySelect/index.tsx (2)
5-9: LGTM! Interface changes are well-structured.
The addition of the optional className prop follows React conventions and TypeScript best practices.
18-31: Verify alpha2ToAlpha3 conversion reliability.
While the code handles potential null values from alpha2ToAlpha3 conversion, we should verify that all country codes are properly converted to prevent unexpected behavior in the sorting logic.
✅ Verification successful
Alpha2ToAlpha3 conversion is properly handled with fallbacks
The code is well-structured with appropriate null handling:
- In
CountrySelect, null values are handled with the nullish coalescing operator (??), defaulting to empty string - In
cashout.utils.ts, there's proper validation with error throwing for invalid country codes - The priority countries list uses alpha-3 codes ('USA', 'CAN') which matches the converted format
The current implementation is safe and handles potential conversion failures gracefully. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all country codes can be properly converted
# Expected: No output indicates all conversions are successful
# Create a temporary file to store the test script
cat << 'EOF' > test_countries.js
const countries = require('i18n-iso-countries');
const countryNames = countries.getNames('en', { select: 'alias' });
Object.keys(countryNames).forEach(code => {
const alpha3 = countries.alpha2ToAlpha3(code);
if (!alpha3) {
console.log(`Warning: Failed to convert country code: ${code}`);
}
});
EOF
# Run the test script if node is available
if command -v node &> /dev/null; then
node test_countries.js
else
echo "Node.js is required to run this verification"
fi
# Cleanup
rm test_countries.js
Length of output: 993
Script:
#!/bin/bash
# Let's verify the actual usage of alpha2ToAlpha3 in the codebase
# and check if there's any error handling or validation elsewhere
# Search for alpha2ToAlpha3 usage
echo "=== Searching for alpha2ToAlpha3 usage ==="
rg "alpha2ToAlpha3" -A 2 -B 2
# Search for any country code validation or error handling
echo -e "\n=== Searching for country code related validation ==="
rg -i "countrycode|country.*validation|validateCountry" -A 2 -B 2
# Look for test files related to country handling
echo -e "\n=== Searching for country-related test files ==="
fd -g "*country*.test.*" -g "*country*.spec.*"
# Check if there are any constants or configurations for country codes
echo -e "\n=== Searching for country code constants or configurations ==="
rg "PRIORITY_COUNTRIES|DEFAULT_COUNTRY" -A 2 -B 2
Length of output: 4069
src/app/api/bridge/liquidation-address/create/route.ts (2)
22-23: LGTM! Headers follow standard conventions.
The header casing follows HTTP header field name conventions and properly indicates JSON communication.
41-58: LGTM! Well-structured error handling.
The error handling is comprehensive with:
- Specific handling for external account mismatch
- Appropriate status codes
- Consistent error response structure
src/components/Global/GeneralRecipientInput/index.tsx (3)
8-8: LGTM: Clean integration of recent recipients functionality
The addition of useRecentRecipients hook and infoText prop aligns well with the PR's objective of enhancing the recipient input experience.
Also applies to: 15-15
36-36: Verify recent recipients hook implementation
The hook integration looks good, but let's verify its implementation.
#!/bin/bash
# Description: Verify the useRecentRecipients hook implementation
# Expected: Find the hook implementation and its test file
# Look for the hook implementation
echo "Looking for useRecentRecipients hook implementation:"
fd "useRecentRecipients.ts" --type f
# Look for the hook tests
echo -e "\nLooking for useRecentRecipients tests:"
fd "useRecentRecipients.test.ts" --type f117-120: LGTM: Enhanced input accessibility and UX
Good additions for accessibility and user experience. The autoComplete and name attributes improve form usability, while suggestions and infoText enhance the user interface.
Note: Ensure that the suggestions feature properly sanitizes user input to prevent XSS attacks.
src/components/Global/ValidatedInput/index.tsx (3)
3-3: LGTM! Props enhance form accessibility and user experience.
The new props (autoComplete, name, suggestions, infoText) and MoreInfo component integration align well with the PR objectives for improving the cashout functionality.
Also applies to: 12-15
89-95: LGTM! Well-implemented UI enhancements.
The addition of the info tooltip and suggestions datalist improves user experience by:
- Providing contextual help through the MoreInfo component
- Offering input suggestions to streamline the cashout process
Also applies to: 115-121
Line range hint 41-76: LGTM! Robust validation implementation.
The validation logic includes:
- Proper cleanup with isStale flag
- Error handling with appropriate user feedback
- Debouncing for performance optimization
src/components/Cashout/Components/Initial.view.tsx (5)
96-98: LGTM! Simplified boolean logic
The direct usage of boolean values improves readability by removing unnecessary double negation.
220-226: LGTM! Well-structured bank account selection handler
The handler follows best practices by:
- Converting account identifiers to lowercase for consistent comparison
- Checking cross-chain allowance
- Properly managing validation states
249-254: LGTM! Clear minimum amount warning
The warning message is well-positioned and provides clear feedback to users about the minimum cashout limit.
292-307: LGTM! Enhanced bank account selection UI
Good improvements:
- Proper accessibility with htmlFor labels
- Consistent case handling in comparisons
- Clear visual feedback for selected state
312-313: LGTM! Consistent case handling in clear button logic
The comparison logic correctly handles case sensitivity using toLowerCase().
src/components/Global/LinkAccountComponent/index.tsx (2)
86-99: Verify routing number extraction logic.
The current implementation assumes the first 9 digits of a US bank account number represent the routing number. This assumption might not be valid for all bank account formats and could lead to incorrect data pre-filling.
Consider adding validation for the extracted routing number:
useEffect(() => {
if (accountNumber && /^[0-9]{6,26}$/.test(accountNumber)) {
// Extract routing number (first 9 digits) and account number (rest)
const routingNumber = accountNumber.slice(0, 9)
+ // Validate routing number format (9 digits)
+ if (!/^[0-9]{9}$/.test(routingNumber)) {
+ console.warn('Invalid routing number format');
+ return;
+ }
const bankAccountNumber = accountNumber.slice(9)
// Pre-fill the form
setAccountDetailsValue('routingNumber', routingNumber)
setAccountDetailsValue('accountNumber', bankAccountNumber)
} else if (accountNumber) {
// If it's an IBAN, just set it as is
setAccountDetailsValue('accountNumber', accountNumber)
}
}, [accountNumber, setAccountDetailsValue])Line range hint 156-164: LGTM! Well-implemented duplicate account detection.
The implementation properly handles case sensitivity and whitespace in account identifiers, with clear error messaging for users.
src/components/Claim/Link/Initial.view.tsx (3)
28-29: LGTM! Import changes align with PR objectives.
The addition of TOOLTIPS and AddressLink imports supports the implementation of addressLink functionality mentioned in the PR objectives.
208-210: LGTM! Improved recipient information handling.
The changes enhance form handling by consistently using the recipient's address as a fallback when the name is not available, ensuring better data consistency.
Also applies to: 215-216
383-385: LGTM! Enhanced address display with AddressLink.
The implementation of AddressLink component improves user experience by making addresses interactive and more readable.
src/components/Global/MoreInfo/index.tsx (1)
21-64: Efficiently manage event listeners in the useEffect hook
The useEffect hook adds event listeners for resize and scroll events. Ensure that these listeners are necessary and do not impact performance, especially if multiple instances of this component might be mounted simultaneously.
Consider confirming that the event listeners are properly cleaned up and do not cause performance issues.
src/components/Global/KYCComponent/index.tsx (10)
3-3: Importing useEffect Hook
Added useEffect to the imports from 'react', which is necessary for the newly implemented side effects in the component.
17-17: Importing useToast from Chakra UI
Added useToast import to handle toast notifications within the component.
52-52: Review Initial kycStatus Value
The kycStatus state is initialized to 'completed'. Consider whether 'completed' is the appropriate initial status. If the KYC process hasn't started yet, initializing it to 'pending' or 'not_started' might better reflect the actual state.
71-71: Initialize toast for Notifications
Initialized toast from Chakra UI to display user notifications.
144-149: Update Iframe Options to Display TOS
Configured iframe options to display the Terms of Service by setting src, enabling visibility, and resetting closeConfirmMessage.
198-205: Display Toast Notification When KYC is Under Review
Appropriately inform users that their KYC is under review by displaying a toast notification.
212-218: Display Toast Notification When KYC is Rejected
Inform users of KYC rejection and prompt them to contact support through a toast notification.
224-232: Set Iframe Options to Display KYC Form
Configured iframe to display the KYC form with appropriate options, including a closeConfirmMessage to alert users about potential progress loss.
235-256: Handle 'KYC_UNDER_REVIEW' Error Appropriately
The code correctly captures the 'KYC_UNDER_REVIEW' error and updates the user interface accordingly, ensuring a smooth user experience.
479-484: Ensure customerId and onKycComplete Are Not Overwritten
When spreading iframeOptions before specifying customerId and onKycComplete, ensure that these keys are not present in iframeOptions to prevent unintentional overwrites.
src/utils/cashout.utils.ts (1)
388-395:
Prevent Potential Infinite Recursion in Error Handling
The recursive call to createLiquidationAddress without a termination condition may lead to potential infinite recursion if the error persists. Consider implementing a maximum retry limit or refactoring to an iterative approach to prevent stack overflows.
Apply this diff to refactor the recursive call:
- return await createLiquidationAddress(
- customerId,
- chainName,
- tokenName,
- matchingAccount.id,
- destinationPaymentRail,
- destinationCurrency
- )
+ // Implement a retry mechanism with a limit or handle the error without recursion
+ throw new Error('Failed to create liquidation address after retrying with the correct external account ID')Likely invalid or redundant comment.
| export const FAQComponent = () => { | ||
| return ( | ||
| <div className={`flex w-full items-center justify-start gap-1 text-left text-h8 ${className}`}> | ||
| FAQ{' '} | ||
| <Menu className="relative" as="div"> | ||
| <> | ||
| <Menu.Button className="flex items-center justify-center"> | ||
| <Icon name={'info'} className={`transition-transform dark:fill-white`} /> | ||
| </Menu.Button> | ||
| <Transition | ||
| enter="transition-opacity duration-150 ease-out" | ||
| enterFrom="opacity-0" | ||
| enterTo="opacity-100" | ||
| leave="transition-opacity duration-100 ease-out" | ||
| leaveFrom="opacity-100" | ||
| leaveTo="opacity-0" | ||
| > | ||
| <Menu.Items className="shadow-primary-4 absolute bottom-full z-[10000] mb-1 mr-1 w-64 border border-n-1 bg-white px-6 py-3 md:left-0 md:right-auto"> | ||
| <Menu.Item as={'div'} className={'text-h8 font-normal'}> | ||
| <p> | ||
| Cashing out requires KYC. | ||
| {/* {' '} | ||
| <Link href="/kyc" className="text-blue-600 underline"> | ||
| KYC | ||
| </Link>. */} | ||
| <br></br>Min cashout: $10, max $100k. | ||
| </p> | ||
| <br></br> | ||
| <p>Fees:</p> | ||
| <ul className="list-disc pl-5"> | ||
| <li>Peanut sponsors gas</li> | ||
| <li>We have to charge a $1 fee for EU cashouts, and $0.5 for US transfers</li> | ||
| </ul> | ||
| <br></br> | ||
| <p>Usually takes 20mins, but can take up to 3 business days.</p> | ||
| <CrispButton className="mt-2 text-blue-600 underline"> | ||
| Need help? Chat with support | ||
| </CrispButton> | ||
| </Menu.Item> | ||
| </Menu.Items> | ||
| </Transition> | ||
| </> | ||
| </Menu> | ||
| <div className="flex w-full items-center justify-start gap-1"> | ||
| FAQ <MoreInfo text={TOOLTIPS.CASHOUT_FAQ} /> | ||
| </div> |
There was a problem hiding this comment.
💡 Codebase verification
FAQ tooltip content needs enhancement with KYC information
The current CASHOUT_FAQ tooltip content covers fees, processing times, and supported currencies, but lacks critical information about KYC requirements and cashout limits that were mentioned in the review. The tooltip should be expanded to include:
- KYC requirements and verification process
- Maximum and minimum cashout limits
🔗 Analysis chain
Verify the completeness of FAQ information in tooltip.
While the simplified implementation improves UI consistency, please ensure that all critical information from the previous menu system (KYC requirements, cashout limits, fees, processing times) is adequately covered in TOOLTIPS.CASHOUT_FAQ.
Additionally, consider these architectural points:
- The removal of the className prop limits component styling flexibility
- The tooltip approach might not be ideal for displaying lengthy FAQ content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the content of TOOLTIPS.CASHOUT_FAQ
# Expected: Should contain comprehensive FAQ information
# Search for TOOLTIPS.CASHOUT_FAQ definition
rg -l "CASHOUT_FAQ.*="
Length of output: 24
Script:
#!/bin/bash
# Let's try different approaches to locate the TOOLTIPS constant and CASHOUT_FAQ
# 1. Search for TOOLTIPS object/constant definition
rg "TOOLTIPS\s*=|TOOLTIPS:\s*{|export\s+const\s+TOOLTIPS" -A 5
# 2. Search specifically for CASHOUT_FAQ in any context
rg "CASHOUT_FAQ" -A 2
# 3. Look for files that might contain tooltip definitions
fd -e ts -e tsx -e js -e jsx | grep -i "tooltip\|constant"
Length of output: 902
Script:
#!/bin/bash
# Let's check the complete content of tooltips.ts to see all FAQ information
cat src/constants/tooltips.ts
Length of output: 1057
| <Link className="cursor-pointer underline" href={url} target="_blank"> | ||
| {utils.printableAddress(address)} | ||
| {displayAddress} | ||
| </Link> | ||
| ) : ( | ||
| <span>{utils.printableAddress(address)}</span> | ||
| <span>{displayAddress}</span> | ||
| ) |
There was a problem hiding this comment.
Add security attributes to external link.
When using target="_blank", it's recommended to add rel="noopener noreferrer" to prevent potential security vulnerabilities.
Apply this change:
-<Link className="cursor-pointer underline" href={url} target="_blank">
+<Link className="cursor-pointer underline" href={url} target="_blank" rel="noopener noreferrer">📝 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.
| <Link className="cursor-pointer underline" href={url} target="_blank"> | |
| {utils.printableAddress(address)} | |
| {displayAddress} | |
| </Link> | |
| ) : ( | |
| <span>{utils.printableAddress(address)}</span> | |
| <span>{displayAddress}</span> | |
| ) | |
| <Link className="cursor-pointer underline" href={url} target="_blank" rel="noopener noreferrer"> | |
| {displayAddress} | |
| </Link> | |
| ) : ( | |
| <span>{displayAddress}</span> | |
| ) |
| {value && ( | ||
| <div className="absolute right-0 top-0 h-full opacity-0 transition-opacity hover:opacity-100"> | ||
| <button | ||
| onClick={(e) => { | ||
| e.preventDefault() | ||
| setValue('') | ||
| }} | ||
| className="flex h-full w-12 items-center justify-center bg-white" | ||
| > | ||
| <Icon className="h-6 w-6 dark:fill-white" name="close" /> | ||
| </button> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and mobile usability of the clear button.
While the clear button implementation is clean and functional, consider these improvements:
{value && (
- <div className="absolute right-0 top-0 h-full opacity-0 transition-opacity hover:opacity-100">
+ <div className="absolute right-0 top-0 h-full sm:opacity-0 sm:transition-opacity sm:hover:opacity-100">
<button
onClick={(e) => {
e.preventDefault()
setValue('')
}}
- className="flex h-full w-12 items-center justify-center bg-white"
+ className="flex h-full w-12 items-center justify-center bg-white focus:outline-none focus:ring-2 focus:ring-primary"
+ aria-label="Clear input"
>
<Icon className="h-6 w-6 dark:fill-white" name="close" />
</button>
</div>
)}Changes:
- Added
aria-labelfor screen readers - Added focus styles for keyboard navigation
- Made the button always visible on mobile (below
smbreakpoint) while keeping hover behavior on desktop
📝 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.
| {value && ( | |
| <div className="absolute right-0 top-0 h-full opacity-0 transition-opacity hover:opacity-100"> | |
| <button | |
| onClick={(e) => { | |
| e.preventDefault() | |
| setValue('') | |
| }} | |
| className="flex h-full w-12 items-center justify-center bg-white" | |
| > | |
| <Icon className="h-6 w-6 dark:fill-white" name="close" /> | |
| </button> | |
| </div> | |
| )} | |
| {value && ( | |
| <div className="absolute right-0 top-0 h-full sm:opacity-0 sm:transition-opacity sm:hover:opacity-100"> | |
| <button | |
| onClick={(e) => { | |
| e.preventDefault() | |
| setValue('') | |
| }} | |
| className="flex h-full w-12 items-center justify-center bg-white focus:outline-none focus:ring-2 focus:ring-primary" | |
| aria-label="Clear input" | |
| > | |
| <Icon className="h-6 w-6 dark:fill-white" name="close" /> | |
| </button> | |
| </div> | |
| )} |
| export const metadata: Metadata = { | ||
| viewport: { | ||
| width: 'device-width', | ||
| initialScale: 1, | ||
| maximumScale: 1, | ||
| userScalable: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Remove restrictions on user scaling for accessibility.
The current viewport configuration prevents users from zooming the page, which creates accessibility issues. Users with visual impairments often need to zoom in to read content comfortably.
Apply this diff to fix the accessibility issue:
export const metadata: Metadata = {
viewport: {
width: 'device-width',
initialScale: 1,
- maximumScale: 1,
- userScalable: false,
+ maximumScale: 5,
+ userScalable: true,
},
}📝 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.
| export const metadata: Metadata = { | |
| viewport: { | |
| width: 'device-width', | |
| initialScale: 1, | |
| maximumScale: 1, | |
| userScalable: false, | |
| }, | |
| } | |
| export const metadata: Metadata = { | |
| viewport: { | |
| width: 'device-width', | |
| initialScale: 1, | |
| maximumScale: 5, | |
| userScalable: true, | |
| }, | |
| } |
| useEffect(() => { | ||
| // Load saved recipients on mount | ||
| const saved = localStorage.getItem(STORAGE_KEY) | ||
| if (saved) { | ||
| setRecentRecipients(JSON.parse(saved)) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Add error handling for localStorage operations.
The JSON.parse operation could throw an error if the stored data is corrupted. Consider adding try-catch block and data validation.
useEffect(() => {
// Load saved recipients on mount
const saved = localStorage.getItem(STORAGE_KEY)
- if (saved) {
- setRecentRecipients(JSON.parse(saved))
- }
+ if (saved) {
+ try {
+ const parsed = JSON.parse(saved)
+ if (Array.isArray(parsed) && parsed.every(item =>
+ typeof item === 'object' &&
+ 'value' in item &&
+ 'type' in item &&
+ 'timestamp' in item
+ )) {
+ setRecentRecipients(parsed)
+ }
+ } catch (error) {
+ console.error('Failed to parse recent recipients:', error)
+ localStorage.removeItem(STORAGE_KEY)
+ }
+ }
}, [])📝 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.
| useEffect(() => { | |
| // Load saved recipients on mount | |
| const saved = localStorage.getItem(STORAGE_KEY) | |
| if (saved) { | |
| setRecentRecipients(JSON.parse(saved)) | |
| } | |
| }, []) | |
| useEffect(() => { | |
| // Load saved recipients on mount | |
| const saved = localStorage.getItem(STORAGE_KEY) | |
| if (saved) { | |
| try { | |
| const parsed = JSON.parse(saved) | |
| if (Array.isArray(parsed) && parsed.every(item => | |
| typeof item === 'object' && | |
| 'value' in item && | |
| 'type' in item && | |
| 'timestamp' in item | |
| )) { | |
| setRecentRecipients(parsed) | |
| } | |
| } catch (error) { | |
| console.error('Failed to parse recent recipients:', error) | |
| localStorage.removeItem(STORAGE_KEY) | |
| } | |
| } | |
| }, []) |
| if (!visible || !customerId) return | ||
|
|
||
| const pollKycStatus = async () => { | ||
| try { | ||
| console.log('🔍 Polling KYC status for customer:', customerId) | ||
| const response = await fetch(`/api/bridge/user/new/get-status`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| userId: customerId, | ||
| type: 'kyc', | ||
| }), | ||
| }) | ||
|
|
||
| if (!response.ok) return | ||
|
|
||
| if (event.origin === `${expectedOrigin}` && event.data === 'close-modal') { | ||
| onClose() | ||
| const data = await response.json() | ||
| const kycStatus = data.kyc_status | ||
| console.log('📊 Current KYC status:', kycStatus) | ||
|
|
||
| if (kycStatus === 'not_started') { | ||
| return // Continue polling without changing isPolling state | ||
| } | ||
|
|
||
| if (kycStatus === 'approved') { | ||
| setIsPolling(false) | ||
| toast({ | ||
| title: 'Success', | ||
| description: 'KYC completed successfully!', | ||
| status: 'success', | ||
| duration: 5000, | ||
| isClosable: true, | ||
| }) | ||
| onKycComplete?.() | ||
| onClose() | ||
| } else if (kycStatus === 'rejected') { | ||
| setIsPolling(false) | ||
| toast({ | ||
| title: 'KYC Rejected', | ||
| description: 'Please contact support.', | ||
| status: 'error', | ||
| duration: 5000, | ||
| isClosable: true, | ||
| }) | ||
| onClose() | ||
| } else if (kycStatus === 'under_review') { | ||
| setIsPolling(false) | ||
| toast({ | ||
| title: 'Under Review', | ||
| description: 'Your KYC is under review. Our team will process it shortly.', | ||
| status: 'info', | ||
| duration: 5000, | ||
| isClosable: true, | ||
| }) | ||
| onClose() | ||
| } | ||
| } catch (error) { | ||
| console.error('❌ Error polling KYC status:', error) | ||
| setIsPolling(false) | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('message', handleMessage) | ||
| let pollInterval: NodeJS.Timeout | ||
|
|
||
| return () => { | ||
| window.removeEventListener('message', handleMessage) | ||
| if (visible && !isPolling) { | ||
| console.log('🔄 Starting KYC status polling...') | ||
| setIsPolling(true) | ||
| // Initial check | ||
| pollKycStatus() | ||
| // Then poll every 3 seconds | ||
| pollInterval = setInterval(pollKycStatus, 3000) | ||
| } | ||
| }, [onClose]) | ||
|
|
||
| const areYouSurePromptRow = showCloseConfirmMessage ? ( | ||
| <div className='flex flex-col text-center sm:text-left sm:flex-row justify-between sm:items-center gap-4 p-2 sm:p-0 w-full'> | ||
| <p className="text-sm ml-1">{closeConfirmMessage}</p> | ||
| <div className="flex flex-row items-center gap-2"> | ||
| <button className='btn-stroke w-full' onClick={() => { | ||
| setShowCloseConfirmMessage(false) | ||
| }}>Cancel</button> | ||
| <button className='btn-purple w-full' onClick={() => { | ||
| onClose() | ||
| setShowCloseConfirmMessage(false) | ||
| }}>Close</button> | ||
| </div> | ||
| </div> | ||
| ) : <button className="btn-purple w-full rounded-none" onClick={() => { | ||
| setShowCloseConfirmMessage(true) | ||
| }}> | ||
| CLOSE | ||
| </button> | ||
| return () => { | ||
| if (pollInterval) { | ||
| console.log('🛑 Stopping KYC status polling') | ||
| clearInterval(pollInterval) | ||
| } | ||
| } | ||
| }, [visible, customerId, onKycComplete, onClose, isPolling, toast]) |
There was a problem hiding this comment.
Fix potential infinite re-renders caused by isPolling in useEffect dependencies
Including isPolling in the dependency array of the useEffect hook can lead to unintended re-invocation of the effect whenever isPolling changes, potentially causing infinite loops or redundant polling. Similarly, including functions like toast, onClose, and onKycComplete may not be necessary if they are stable references.
Apply this diff to adjust the dependency array:
- }, [visible, customerId, onKycComplete, onClose, isPolling, toast])
+ }, [visible, customerId])Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| // Listen for messages from the iframe | ||
| const handleMessage = (event: MessageEvent) => { | ||
| // Add origin check for security | ||
| if (event.origin !== window.location.origin) return | ||
|
|
||
| if (event.data.type === 'PERSONA_COMPLETE') { | ||
| console.log('KYC completed successfully') | ||
| // Add mobile-specific logging | ||
| console.log('Device info:', { | ||
| isMobile: window.innerWidth < 768, | ||
| windowSize: `${window.innerWidth}x${window.innerHeight}`, | ||
| userAgent: navigator.userAgent, | ||
| }) | ||
|
|
||
| // Ensure the state updates happen | ||
| setKycStatus('completed') | ||
| if (onCompleted) { | ||
| onCompleted('KYC completed') | ||
| } | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('message', handleMessage) | ||
| return () => window.removeEventListener('message', handleMessage) | ||
| }, [onCompleted]) |
There was a problem hiding this comment.
Review Origin Check in handleMessage Event Listener
In the handleMessage function, the check if (event.origin !== window.location.origin) return may prevent messages from the iframe if it is hosted on a different domain. Since iframes often come from third-party domains (e.g., Persona for KYC), consider updating the origin check to allow messages from the iframe's origin or implement a more secure verification method.
Avoid Logging Sensitive User Information in Production
The console.log statements within handleMessage log device information, including the user agent and window size. While useful for debugging, consider removing these logs or wrapping them in a condition that ensures they only execute in a development environment to protect user privacy.
| const allAccounts = await fetch(`/api/bridge/external-account/get-all-for-customerId`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| customerId, | ||
| }), | ||
| }) | ||
|
|
||
| if (!allAccounts.ok) { | ||
| throw new Error('Failed to fetch existing accounts') | ||
| } | ||
|
|
||
| const accounts = await allAccounts.json() | ||
| // Find the matching account based on account details |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Fetching of Existing Accounts
Fetching all external accounts and then filtering on the client side may be inefficient and could expose unnecessary data. Consider modifying the API to allow querying for the specific external account based on unique identifiers to improve performance and reduce data exposure.
|
|
||
| const accountsData = await accountsResponse.json() | ||
|
|
||
| // Ensure we have an array of accounts |
There was a problem hiding this comment.
Ensure Accurate Matching of External Accounts
The current logic may select any account that matches the account_type, which might not be the intended external account. To ensure the correct external account is selected, consider refining the matching criteria by comparing additional unique identifiers, such as account_details, accountOwnerName, or other specific attributes.
| matchingAccount.id, | ||
| destinationPaymentRail, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Logging Raw Error Objects
Logging raw error objects may inadvertently expose sensitive information. Consider sanitizing error messages before logging to prevent potential leakage of sensitive data.
| } else { | ||
| setDisplayAddress(utils.printableAddress(address)) | ||
| } | ||
| }, [address, ensName]) |
There was a problem hiding this comment.
nit: I feel like there's no real need for effects or state here. We're just displaying conditionally either the ens is present or not
There was a problem hiding this comment.
no cuz we do:
0x --> fetch if ens exists --> ens
A lot of changes.
TODO: no way of both suggesting IBAN AND US accounts. Need to rework this flow in the future to also suggest browser us accounts if possible.