[TASK-6748] [TASK-7307] [TASK-7306] fix: qa session issues#562
[TASK-6748] [TASK-7307] [TASK-7306] fix: qa session issues#562jjramirezn merged 16 commits intopeanut-walletfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces updates across various components, focusing on wallet management and user interface enhancements. Notable changes include the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/components/Home/WalletCard.tsx (1)
82-90: Add feedback for clipboard operationsWhile the copy functionality is a good addition, users should receive feedback when the address is copied.
Consider adding a toast notification:
onClick={(e: MouseEvent<SVGElement>) => { e.stopPropagation() - navigator.clipboard.writeText(wallet.address) + navigator.clipboard.writeText(wallet.address) + .then(() => { + // Assuming you have a toast notification system + toast.success('Address copied to clipboard') + }) + .catch((error) => { + console.error('Failed to copy address:', error) + toast.error('Failed to copy address') + }) }}src/app/(mobile-ui)/wallet/page.tsx (2)
43-43: Consider adding loading state for balanceThe balance handling with nullish coalescing is good, but consider showing a loading state while the balance is being fetched.
-<div className="text-5xl">$ {printableUsdc(selectedWallet?.balance ?? 0n)}</div> +<div className="text-5xl"> + {selectedWallet === undefined ? ( + <span className="animate-pulse">Loading...</span> + ) : ( + `$ ${printableUsdc(selectedWallet?.balance ?? 0n)}` + )} +</div>
Line range hint
46-64: Consider disabling cashout button based on balanceThe cashout button should be disabled when the balance is 0 or when the wallet is not connected.
-<Button shadowSize="4"> +<Button + shadowSize="4" + disabled={!selectedWallet?.connected || (selectedWallet?.balance ?? 0n) <= 0n} +> <Link href="/cashout" className="flex flex-row items-center text-nowrap">src/utils/balance.utils.ts (1)
106-108: Consider adding input validation for negative amountsThe function correctly formats USDC amounts using the standard 6 decimals. However, consider adding validation for negative amounts to ensure consistent display.
export function printableUsdc(baseUnitsAmount: bigint): string { + if (baseUnitsAmount < 0n) { + throw new Error('Negative amounts are not supported') + } return Number(formatUnits(baseUnitsAmount, 6)).toFixed(2) }src/components/Cashout/Components/Initial.view.tsx (1)
Line range hint
12-450: Consider extracting bank account validation logicThe component handles multiple responsibilities including bank account validation, KYC flow, and balance management. Consider extracting the bank account validation logic into a custom hook for better maintainability and reusability.
Example structure:
// hooks/useBankAccountValidation.ts export function useBankAccountValidation() { const [bankAccountNumber, setBankAccountNumber] = useState<string>('') const [isValid, setIsValid] = useState<boolean>(false) const [isValidating, setIsValidating] = useState<boolean>(false) const handleValidation = useCallback(/* validation logic */) const formatDisplay = useCallback(/* formatting logic */) return { bankAccountNumber, setBankAccountNumber, isValid, isValidating, handleValidation, formatDisplay } }src/components/Global/Icon/index.tsx (1)
Line range hint
1-213: Consider restructuring icon management for better maintainabilityAs the icon collection grows, consider:
- Splitting icons into separate files by category
- Implementing dynamic icon loading
- Using an icon management system or design system
This would improve maintainability and potentially reduce the bundle size through code-splitting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
src/app/(mobile-ui)/layout.tsx(3 hunks)src/app/(mobile-ui)/wallet/page.tsx(2 hunks)src/components/Cashout/Components/Initial.view.tsx(2 hunks)src/components/Create/Link/Input.view.tsx(2 hunks)src/components/Global/Icon/index.tsx(3 hunks)src/components/Home/HomeHeader.tsx(2 hunks)src/components/Home/WalletCard.tsx(2 hunks)src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx(1 hunks)src/context/walletContext/walletContext.tsx(3 hunks)src/utils/balance.utils.ts(2 hunks)
🧰 Additional context used
📓 Learnings (2)
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#412
File: src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx:37-37
Timestamp: 2024-11-12T09:39:20.720Z
Learning: The files 'src/components/Claim/Generic/AlreadyClaimed.view.tsx' and 'NotFound.view.tsx' are part of the send flow, so the link text 'Make a payment yourself!' and href '/send' should remain as they are.
src/components/Create/Link/Input.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#495
File: src/components/Create/Link/Input.view.tsx:244-248
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `TokenAmountInput` component within `src/components/Global/TokenAmountInput/index.tsx`, when `balance` is undefined, the `maxValue` prop should be set to an empty string `''`.
🔇 Additional comments (8)
src/components/Home/HomeHeader.tsx (1)
9-9: Verify wallet connection state handling
The changes improve wallet connection state management by using dedicated flags (isPeanutWallet, isConnected) instead of relying on selectedWallet?.connected. However, we should verify the behavior when switching between different wallet types.
Also applies to: 29-41
✅ Verification successful
Wallet connection state handling is correctly implemented
The changes in HomeHeader.tsx align perfectly with the wallet context implementation. The context provides isConnected and isPeanutWallet flags that are properly derived:
isConnectedis set based onselectedWallet?.connectedisPeanutWalletis determined by theisPeanuthelper function- Connection state is consistently managed through the
isWalletConnectedcallback
The new approach in HomeHeader.tsx using these dedicated flags is more robust than directly accessing selectedWallet?.connected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet connection state handling across the codebase
# to ensure consistent behavior
# Check wallet state management in context
ast-grep --pattern 'const { $$$, isPeanutWallet, isConnected } = useWallet()'
# Check other components using these flags
rg "isPeanutWallet|isConnected" --type tsx
Length of output: 270
Script:
#!/bin/bash
# Search for wallet connection state handling with correct file extension
rg "isPeanutWallet|isConnected" -t ts -t tsx
# Check useWallet hook implementation
fd "useWallet" -e ts -e tsx --exec cat {}
# Look for wallet connection state patterns
ast-grep --pattern 'selectedWallet?.connected'
Length of output: 2405
Script:
#!/bin/bash
# Check the wallet context implementation
fd "walletContext" -e ts -e tsx --exec cat {}
# Look for components using selectedWallet.connected
rg "selectedWallet.*connected" -t ts -t tsx
Length of output: 10534
src/components/Home/WalletCard.tsx (1)
77-77: LGTM: Improved balance formatting
The switch to printableUsdc provides consistent USDC balance formatting across the application.
src/components/Request/Pay/Views/GeneralViews/AlreadyPaid.view.tsx (1)
24-24: LGTM! Text change improves clarity
The updated text "This link previously contained:" better describes the content that follows, making the UI more intuitive.
src/context/walletContext/walletContext.tsx (1)
36-36: LGTM: Clean implementation of external wallet selection
The new selectExternalWallet method is well-implemented with proper memoization and follows React best practices. The change aligns with the PR objective of improving wallet connect handling.
Also applies to: 238-240, 260-260
src/components/Create/Link/Input.view.tsx (1)
11-11: LGTM: Consistent balance formatting with proper undefined handling
The implementation correctly uses printableUsdc for balance formatting and properly handles undefined balance as per the learning from PR #495.
Also applies to: 269-269
src/components/Cashout/Components/Initial.view.tsx (1)
12-12: LGTM: Consistent balance formatting implementation
The implementation of printableUsdc for balance formatting maintains consistency with other components.
Also applies to: 200-200
src/components/Global/Icon/index.tsx (2)
213-213: LGTM: Enhanced onClick type signature
The updated type signature for onClick now includes the mouse event object, providing better TypeScript support for event handling.
77-78: LGTM: Added content-copy icon
The new icon addition is well-formatted and aligns with the PR objective of adding copy functionality to wallet cards.
|
|
||
| export function printableUsdc(baseUnitsAmount: bigint): string { | ||
| return Number(formatUnits(baseUnitsAmount, 6)).toFixed(2) | ||
| } |
There was a problem hiding this comment.
Thought: seems a bit hardcoded
There was a problem hiding this comment.
I think is ok for now, PW is only usdc and usdc is 6 decimals
| <Card className="shadow-none sm:shadow-primary-4"> | ||
| <Card.Header className="text-center"> | ||
| <Card.Title>Sorry, this link has already been paid.</Card.Title> | ||
| <Card.Title>This link previously contained:</Card.Title> |
There was a problem hiding this comment.
In main this line is like this:
<label className="text-h2">Payment Receipt</label>
We should keep that and not overwrite!
| <Icon | ||
| name="content-copy" | ||
| className="absolute bottom-0 right-0 h-5 w-5 hover:opacity-80" | ||
| fill="white" | ||
| onClick={(e: MouseEvent<SVGElement>) => { | ||
| e.stopPropagation() | ||
| navigator.clipboard.writeText(wallet.address) | ||
| }} | ||
| /> |
There was a problem hiding this comment.
In my opinion this is bad from a UX perspective - you click the copy thing, but nothing happens, visually. It's not clear that anything was copied.
We should make it clear for the user that smth happened. I'll make a discord thread
|
Behavioural QA on https://peanut-6590depp0-squirrellabs.vercel.app/request/create:
|
|
Added some requested fixes
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/app/manifest.ts (1)
5-6: Consider extracting environment-specific names for better readability.The ternary operators make the code less maintainable. Consider extracting these into a constant.
+const APP_NAME = process.env.NODE_ENV === 'development' ? 'Peanut Dev' : 'Peanut'; + export default function manifest(): MetadataRoute.Manifest { return { - name: process.env.NODE_ENV === 'development' ? 'Peanut Dev' : 'Peanut', - short_name: process.env.NODE_ENV === 'development' ? 'Peanut Dev' : 'Peanut', + name: APP_NAME, + short_name: APP_NAME,src/components/Claim/Link/Initial.view.tsx (1)
466-487: Clean up and improve the GeneralRecipientInput implementation.A few suggestions to improve the code:
- Remove the empty className prop.
- Consider extracting the onUpdate callback to a separate function for better readability.
- Make the recipient type handling more explicit with a type guard.
{isExternalWallet && ( <GeneralRecipientInput - className="" placeholder="wallet address / ENS / IBAN / US account number" recipient={recipient} - onUpdate={(update: GeneralRecipientUpdate) => { - setRecipient(update.recipient) - if (!update.recipient.address) { - setRecipientType('address') - } else { - setRecipientType(update.type) - } - setIsValidRecipient(update.isValid) - setErrorState({ - showError: !update.isValid, - errorMessage: update.errorMessage, - }) - setInputChanging(update.isChanging) - }} + onUpdate={handleRecipientUpdate} infoText={TOOLTIPS.CLAIM_RECIPIENT_INFO} /> )}Add this function above the return statement:
const handleRecipientUpdate = useCallback((update: GeneralRecipientUpdate) => { setRecipient(update.recipient); setRecipientType(update.recipient.address ? update.type : 'address'); setIsValidRecipient(update.isValid); setErrorState({ showError: !update.isValid, errorMessage: update.errorMessage, }); setInputChanging(update.isChanging); }, []);src/components/Create/useCreateLink.tsx (1)
579-579: Ensure 'handleSendUserOpEncoded' is properly memoizedBy adding
handleSendUserOpEncodedto the dependency array of thesendTransactionsuseCallback, you should ensure thathandleSendUserOpEncodedis memoized usinguseCallback. This prevents unnecessary re-creations of thesendTransactionsfunction whenhandleSendUserOpEncodeddoesn't actually change, improving performance.src/app/api/peanut/user/add-account/route.ts (1)
37-38: Clarify the error message for existing accountsThe error message
"User already exists"might be misleading since this endpoint is for adding an account to an existing user. Consider changing the error message to"Account already exists"to provide clearer feedback.src/context/walletContext/zeroDevContext.context.tsx (1)
206-237: Enhanced handleSendUserOpEncoded with better error handling and performance.The changes improve the function by:
- Adding memoization with useCallback to prevent unnecessary re-renders
- Adding client initialization check
- Improving error handling for user operations
However, there are a few potential improvements:
- Consider adding timeout handling for user operations
- Add error handling for receipt waiting
const handleSendUserOpEncoded = useCallback( async ({ to, value, data }: UserOpEncodedParams) => { if (!kernelClient) { throw new Error('Trying to send user operation before client initialization') } setIsSendingUserOp(true) + try { const userOpHash = await kernelClient.sendUserOperation({ account: kernelClient.account, callData: await kernelClient.account!.encodeCalls([ { to: (to ? to : '') as `0x${string}`, value: value ? BigInt(value.toString()) : BigInt(0), data, }, ]), }) const receipt = await kernelClient.waitForUserOperationReceipt({ hash: userOpHash, + timeout: 60000, // 1 minute timeout }) console.log({ receipt }) const userOpMessage = `UserOp completed. <a href="https://jiffyscan.xyz/userOpHash/${userOpHash}?network=sepolia" target="_blank" rel="noopener noreferrer" class="text-blue-500 hover:text-blue-700">Click here to view.</a>` console.log({ userOpMessage }) return receipt.receipt.transactionHash + } catch (error) { + console.error('Error in handleSendUserOpEncoded:', error) + throw error + } finally { setIsSendingUserOp(false) + } }, [kernelClient] )src/components/Request/Pay/Views/Initial.view.tsx (1)
479-494: Improved wallet connection handling with better error management.The changes enhance the user experience by:
- Differentiating between Peanut wallet and external wallet connections
- Adding loading state management
- Providing error feedback through toast notifications
However, there's room for improvement:
Consider extracting the wallet connection logic into a separate function for better maintainability:
+const handleWalletConnection = async ( + isPeanutWallet: boolean, + handleLogin: () => Promise<void>, + setLoadingState: (state: string) => void, + signInModal: { open: () => void }, + toast: { error: (message: string) => void } +) => { + if (isPeanutWallet) { + setLoadingState('Logging in') + try { + await handleLogin() + } catch (error) { + toast.error('Error logging in') + console.error('Login error:', error) + } finally { + setLoadingState('Idle') + } + } else { + signInModal.open() + } +} <Button disabled={isButtonDisabled} onClick={() => { if (!isConnected) { - if (isPeanutWallet) { - setLoadingState('Logging in') - handleLogin() - .catch((_error) => { - toast.error('Error logging in') - }) - .finally(() => { - setLoadingState('Idle') - }) - } else { - signInModal.open() - } + handleWalletConnection(isPeanutWallet, handleLogin, setLoadingState, signInModal, toast) } else if (ViewState.READY_TO_PAY === viewState) { handleOnNext() } }} loading={viewState === ViewState.LOADING} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
src/app/api/peanut/user/add-account/route.ts(1 hunks)src/app/manifest.ts(2 hunks)src/components/Claim/Link/Initial.view.tsx(2 hunks)src/components/Create/Link/Input.view.tsx(5 hunks)src/components/Create/useCreateLink.tsx(1 hunks)src/components/Home/HomeHeader.tsx(2 hunks)src/components/Request/Create/Views/Initial.view.tsx(2 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)src/context/authContext.tsx(1 hunks)src/context/walletContext/walletContext.tsx(6 hunks)src/context/walletContext/zeroDevContext.context.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Home/HomeHeader.tsx
- src/components/Create/Link/Input.view.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/Request/Create/Views/Initial.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#551
File: src/components/Request/Create/Views/Initial.view.tsx:151-156
Timestamp: 2024-12-02T17:19:18.532Z
Learning: In the `InitialView` component at `src/components/Request/Create/Views/Initial.view.tsx`, when setting the default chain and token in the `useEffect` triggered by `isPeanutWallet`, it's acceptable to omit the setters from the dependency array and not include additional error handling for invalid defaults.
src/components/Claim/Link/Initial.view.tsx (1)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#545
File: src/components/Global/GeneralRecipientInput/index.tsx:118-126
Timestamp: 2024-11-26T12:06:11.603Z
Learning: For bank account input fields, use `autocomplete="bank-account-number"` when the recipient type is `'us'` or `'iban'`.
🔇 Additional comments (5)
src/app/manifest.ts (1)
31-31: Verify the security implications of root scope.
Setting the scope to '/' allows the PWA to control all pages under the root path. While this might be necessary for wallet functionality, please verify:
- If this broad scope is intentional
- If there are any security implications for wallet operations
- If a more restrictive scope would suffice
✅ Verification successful
Let me gather more specific information about the PWA and service worker configuration to better assess the security implications of the root scope.
Let me check one more aspect to understand the complete PWA setup and its relationship with the wallet functionality.
Root scope is necessary and properly implemented for this PWA wallet application
The root scope (/) is appropriate and necessary in this case because:
- The app is a PWA wallet application that needs system-wide functionality for protocol handlers (
web+peanut) - The service worker registration in
pushProvider.tsxmatches this scope for consistent push notifications - The manifest's
start_urlis set to/homewith proper security headers in place - The middleware implements appropriate security controls for API routes
The broad scope is intentionally designed to support the wallet's core functionality while maintaining security through:
- Proper CORS and security headers configuration in
next.config.js - API route protection with cache control headers
- Scoped protocol handlers for the
web+peanutscheme
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for route handlers and API endpoints that might be affected by the root scope
# Look for Next.js API routes and handlers
echo "Checking for API routes and handlers..."
fd --type f "route\.(ts|js|tsx|jsx)$" "app"
# Look for sensitive paths that might need protection
echo "Checking for sensitive paths..."
rg -l "api|auth|wallet|private" --type ts --type js
Length of output: 5803
Script:
#!/bin/bash
# Check PWA and service worker configuration
echo "Checking PWA configuration..."
rg -A 10 "manifest = {" src/
# Check service worker registration and scope usage
echo "Checking service worker configuration..."
rg -A 5 "serviceWorker|navigator\.service" --type ts --type js
# Check for security-related configurations
echo "Checking security headers and middleware..."
rg -A 5 "headers|security|cors" src/middleware.ts
# Look for the actual manifest usage
echo "Checking manifest usage..."
rg -C 5 "manifest" next.config.js
Length of output: 2607
Script:
#!/bin/bash
# Check the manifest file content
echo "Checking manifest file content..."
rg -l "manifest\.(ts|js|json)" src/
cat src/app/manifest.ts
# Check wallet-related routes and functionality
echo "Checking wallet-related routes..."
rg -C 5 "wallet" src/app/\(mobile-ui\)/wallet/page.tsx
# Check service worker file
echo "Checking service worker implementation..."
fd "sw.js"
Length of output: 3277
src/components/Claim/Link/Initial.view.tsx (1)
Line range hint 1-24: Consider addressing TODOs and improving component structure.
- The TODO comment about wallet connection handling should be addressed as it's directly related to the PR objectives.
- The component is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more focused components:
- WalletConnectionHandler
- ClaimLinkForm
- RecipientInput
- RouteDisplay
Let's verify the wallet connection handling across the codebase:
src/context/authContext.tsx (1)
220-241: Ensure proper error handling flow in addAccount
The updated error handling in the addAccount method improves clarity by directly managing specific status codes. The implementation looks correct and should handle the 409 status appropriately.
src/components/Request/Create/Views/Initial.view.tsx (1)
158-164: Initialization of recipient address for Peanut Wallet
The useEffect hook correctly sets the recipient address and validation state when using a Peanut wallet. This ensures that the recipient address defaults to the user's own address in this context.
src/context/walletContext/walletContext.tsx (1)
184-190: Improved error handling for BYOW wallet addition.
The error handling now provides clear user feedback through toast notifications, distinguishing between duplicate accounts and unexpected errors.
| useEffect(() => { | ||
| if (isPeanutWallet && address) { | ||
| setRecipient({ name: undefined, address: address }) | ||
| setIsValidRecipient(true) | ||
| } | ||
| }, [isPeanutWallet, address]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential side effects of the useEffect hook.
The hook automatically sets the recipient address for Peanut wallet users, but there are a few concerns:
- The hook might override user input if they're using a Peanut wallet.
- The
setRecipientandsetIsValidRecipientfunctions should be included in the dependency array to prevent stale closures.
useEffect(() => {
if (isPeanutWallet && address) {
setRecipient({ name: undefined, address: address })
setIsValidRecipient(true)
}
- }, [isPeanutWallet, address])
+ }, [isPeanutWallet, address, setRecipient, setIsValidRecipient])📝 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(() => { | |
| if (isPeanutWallet && address) { | |
| setRecipient({ name: undefined, address: address }) | |
| setIsValidRecipient(true) | |
| } | |
| }, [isPeanutWallet, address]) | |
| useEffect(() => { | |
| if (isPeanutWallet && address) { | |
| setRecipient({ name: undefined, address: address }) | |
| setIsValidRecipient(true) | |
| } | |
| }, [isPeanutWallet, address, setRecipient, setIsValidRecipient]) |
| const selectExternalWallet = useCallback(() => { | ||
| setSelectedAddress(wagmiAddress) | ||
| }, [wagmiAddress]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to selectExternalWallet.
The selectExternalWallet implementation should handle the case where wagmiAddress is undefined to prevent potential runtime errors.
const selectExternalWallet = useCallback(() => {
+ if (!wagmiAddress) {
+ console.error('Cannot select external wallet: wagmiAddress is undefined')
+ return
+ }
setSelectedAddress(wagmiAddress)
}, [wagmiAddress])📝 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 selectExternalWallet = useCallback(() => { | |
| setSelectedAddress(wagmiAddress) | |
| }, [wagmiAddress]) | |
| const selectExternalWallet = useCallback(() => { | |
| if (!wagmiAddress) { | |
| console.error('Cannot select external wallet: wagmiAddress is undefined') | |
| return | |
| } | |
| setSelectedAddress(wagmiAddress) | |
| }, [wagmiAddress]) |
|
Added error when trying to claim not supported token |
| .open() | ||
| .then(selectExternalWallet) | ||
| .catch((e) => { | ||
| console.error(e) | ||
| toast.error('Error connecting wallet') | ||
| }) | ||
| .finally(signInModal.close) |
| @@ -27,19 +28,21 @@ const HomeHeader = () => { | |||
| <p>www.peanut.me/</p> | |||
| <p className="text-h4">{username}</p> | |||
There was a problem hiding this comment.
Not sure i understand the logic here
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/context/walletContext/zeroDevContext.context.tsx (1)
69-69: Add type safety for useAuth hookThe destructured values from useAuth lack proper typing.
-const { fetchUser, user } = useAuth() +const { fetchUser, user } = useAuth() as { + fetchUser: () => Promise<void>; + user: { + user?: { + username?: string; + }; + } | null; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/app/api/proxy/[...slug]/route.ts(1 hunks)src/components/Home/HomeHeader.tsx(2 hunks)src/context/walletContext/zeroDevContext.context.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Home/HomeHeader.tsx
🔇 Additional comments (3)
src/context/walletContext/zeroDevContext.context.tsx (1)
210-214: LGTM! Good improvements to handleSendUserOpEncoded
The changes improve the code in two ways:
- Using useCallback prevents unnecessary re-renders
- Adding a null check for kernelClient prevents runtime errors
Also applies to: 240-241
src/app/api/proxy/[...slug]/route.ts (2)
Line range hint 1-62: Consider enhancing security and error handling
While reviewing the proxy implementation, here are some additional security considerations:
-
Error Handling:
- Add specific error types and messages for different failure scenarios
- Include request correlation IDs for better debugging
- Consider logging sensitive operations (with appropriate data masking)
-
API Key Security:
- Validate the API key format before use
- Consider using environment validation at startup
- Add rate limiting to prevent abuse
Here's a suggested implementation for improved error handling:
+ import { rateLimit } from '@/utils/rate-limit'
+ import { v4 as uuidv4 } from 'uuid'
+ const limiter = rateLimit({
+ interval: 60 * 1000, // 1 minute
+ uniqueTokenPerInterval: 500
+ })
export async function POST(request: NextRequest) {
+ const correlationId = uuidv4()
+
+ try {
+ // Rate limiting
+ await limiter.check(request, 100, 'CACHE_TOKEN') // 100 requests per minute
+ } catch {
+ return NextResponse.json({
+ error: 'Too many requests',
+ correlationId
+ }, { status: 429 })
+ }
// Validate API key
+ if (!process.env.PEANUT_API_KEY || !/^[A-Za-z0-9-_]+$/.test(process.env.PEANUT_API_KEY)) {
+ console.error(`[${correlationId}] Invalid API key configuration`)
+ return NextResponse.json({
+ error: 'Internal server error',
+ correlationId
+ }, { status: 500 })
+ }
// ... rest of the code ...
catch (error: any) {
console.error(`[${correlationId}] Error:`, error)
return NextResponse.json({
error: 'Internal server error',
correlationId
}, { status: 500 })
}
}Let's check for existing rate limiting implementations and error handling patterns:
38-40:
Add validation for the x-username header
While the implementation is functionally correct, there are several security considerations to address:
- The code forwards the x-username header without validation, which could potentially be exploited for header injection attacks.
- Consider adding validation to ensure the username:
- Has a maximum length
- Contains only allowed characters
- Follows expected format
- If the username contains PII data, ensure proper handling according to data protection requirements.
Here's a suggested implementation with validation:
- if (request.headers.get('x-username')) {
- headersToPass['x-username'] = request.headers.get('x-username')
- }
+ const username = request.headers.get('x-username')
+ if (username) {
+ // Validate username format and length
+ if (username.length > 100) {
+ return NextResponse.json('Username too long', { status: 400 })
+ }
+ // Basic validation - adjust regex pattern based on your requirements
+ if (!/^[a-zA-Z0-9_-]+$/.test(username)) {
+ return NextResponse.json('Invalid username format', { status: 400 })
+ }
+ headersToPass['x-username'] = username
+ }Let's check if there are any security guidelines or similar header validations in the codebase:
| const passkeyServerHeaders: Record<string, string> = {} | ||
| if (user?.user?.username) { | ||
| passkeyServerHeaders['x-username'] = user.user.username | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add toast notifications for login failures
Based on the PR comments about wallet connection issues, we should enhance error handling to provide better user feedback.
const handleLogin = async () => {
setIsLoggingIn(true)
try {
const passkeyServerHeaders: Record<string, string> = {}
if (user?.user?.username) {
passkeyServerHeaders['x-username'] = user.user.username
}
// ... existing code ...
} catch (e) {
console.error('Error logging in', e)
+ toast.error('Failed to connect wallet. Please try again.')
throw e
} finally {
await fetchUser()
setIsLoggingIn(false)
}
}Also applies to: 183-183
| const userOpMessage = `UserOp completed. <a href="https://jiffyscan.xyz/userOpHash/${userOpHash}?network=sepolia" target="_blank" rel="noopener noreferrer" class="text-blue-500 hover:text-blue-700">Click here to view.</a>` | ||
| console.log({ userOpMessage }) |
There was a problem hiding this comment.
Potential XSS vulnerability in userOpMessage
The HTML in the template string could be dangerous if rendered directly in the UI.
Consider either:
- Using a safe link component from your UI library
- Sanitizing the HTML before rendering
-const userOpMessage = `UserOp completed. <a href="https://jiffyscan.xyz/userOpHash/${userOpHash}?network=sepolia" target="_blank" rel="noopener noreferrer" class="text-blue-500 hover:text-blue-700">Click here to view.</a>`
+// Option 1: Split into text and URL for safe rendering in UI
+const userOpMessage = {
+ text: 'UserOp completed.',
+ link: `https://jiffyscan.xyz/userOpHash/${userOpHash}?network=sepolia`
+}Committable suggestion skipped: line range outside the PR's diff.
2 participants
This PR contains several bugfixes and improvements
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style