[TASK-14388] Feat/request on ramp improvements#1207
[TASK-14388] Feat/request on ramp improvements#1207Zishan-7 merged 7 commits intopeanut-wallet-devfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds URL-driven entry into the bank fulfillment flow, introduces BankRequestType detection and new exports/hooks, consolidates user name fields into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Request/views/ReqFulfillBankFlowManager.tsx (1)
147-156: Form initialData no longer matches updated UserDetailsForm shape (fullName only).The form won’t prefill the name. Provide
fullNameinstead offirstName/lastName.- const [firstName, ...lastNameParts] = (user?.user.fullName ?? '').split(' ') - const lastName = lastNameParts.join(' ') - - const initialUserDetails: Partial<UserDetailsFormData> = useMemo( - () => ({ - firstName: user?.user.fullName ? firstName : '', - lastName: user?.user.fullName ? lastName : '', - email: user?.user.email ?? '', - }), - [user?.user.fullName, user?.user.email, firstName, lastName] - ) + const initialUserDetails: Partial<UserDetailsFormData> = useMemo( + () => ({ + fullName: user?.user.fullName ?? '', + email: user?.user.email ?? '', + }), + [user?.user.fullName, user?.user.email] + )src/components/Global/PostSignupActionManager/index.tsx (1)
33-39: Sanitize redirect before router.push to prevent open-redirect/navigation to external origins.
redirectUrlis pushed verbatim and can be absolute (e.g., “https://...”), enabling navigation off-site. Parse and enforce same-origin or require a leading “/”.Apply:
- action: () => { - router.push(redirectUrl) - localStorage.removeItem('redirect') - setShowModal(false) - }, + action: () => { + const safeUrl = toSafePath(redirectUrl) + router.push(safeUrl) + localStorage.removeItem('redirect') + setShowModal(false) + },Add helper (same file, top-level):
function toSafePath(url: string): string { try { const u = new URL(url, window.location.origin) if (u.origin !== window.location.origin) return '/' return `${u.pathname}${u.search}${u.hash}` } catch { return typeof url === 'string' && url.startsWith('/') ? url : '/' } }
🧹 Nitpick comments (7)
src/components/Kyc/index.tsx (1)
29-35: Save redirect before initiating KYC to avoid losing context on provider redirects.If
handleInitiateKyc()triggers a navigation, saving after success may be too late. MovesaveRedirectUrl()before the initiation.const handleVerifyClick = async () => { - addParamStep('bank') - const result = await handleInitiateKyc() - if (result?.success) { - saveRedirectUrl() - onClose() - } + addParamStep('bank') + saveRedirectUrl() + const result = await handleInitiateKyc() + if (result?.success) { + onClose() + } }src/components/AddMoney/UserDetailsForm.tsx (2)
95-103: Use native email input type for better UX and platform validation.- {renderInput('email', 'E-mail', { + {renderInput('email', 'E-mail', { required: 'Email is required', pattern: { value: /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i, message: 'Invalid email address', }, - })} + }, 'email')}
39-47: Trim inputs before submit to avoid trailing/leading whitespace in name/email.useImperativeHandle(ref, () => ({ - handleSubmit: handleSubmit(async (data) => { + handleSubmit: handleSubmit(async (data) => { setSubmissionError(null) - const result = await onSubmit(data) + const result = await onSubmit({ + ...data, + fullName: data.fullName.trim(), + email: data.email.trim(), + }) if (result?.error) { setSubmissionError(result.error) } }), }))- onSubmit={(e) => { + onSubmit={(e) => { e.preventDefault() - handleSubmit(async (data) => { + handleSubmit(async (data) => { setSubmissionError(null) - const result = await onSubmit(data) + const result = await onSubmit({ + ...data, + fullName: data.fullName.trim(), + email: data.email.trim(), + }) if (result?.error) { setSubmissionError(result.error) } })() }}Also applies to: 82-91
src/components/Request/views/ReqFulfillBankFlowManager.tsx (1)
58-58: Remove stray debug log.- console.log('first')src/components/Profile/views/IdentityVerification.view.tsx (1)
31-40: Remove unused name-splitting and clean dependencies.These vars are no longer needed with
fullName. Keeps lint quiet and memo accurate.- const [firstName, ...lastNameParts] = (user?.user.fullName ?? '').split(' ') - const lastName = lastNameParts.join(' ') - const initialUserDetails: Partial<UserDetailsFormData> = useMemo( () => ({ fullName: user?.user.fullName ?? '', email: user?.user.email ?? '', }), - [user?.user.fullName, user?.user.email, firstName, lastName] + [user?.user.fullName, user?.user.email] )src/components/Global/PostSignupActionManager/post-signup-action.consts.ts (1)
15-15: Fix typo in comment.“determing” → “determining”.
- // this regex will match any path that resembles the request link, this helps in determing if the user is coming from a request link + // this regex will match any path that resembles the request link, this helps in determining if the user is coming from a request linksrc/components/Global/PostSignupActionManager/index.tsx (1)
17-23: Tighten types for actionConfig.icon.Use
IconNameinstead ofanyto catch mismatches at compile time.- const [actionConfig, setActionConfig] = useState<{ + const [actionConfig, setActionConfig] = useState<{ cta: string title: string description: string - icon: any + icon: IconName action: () => void } | null>(null)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/[...recipient]/client.tsx(3 hunks)src/components/AddMoney/UserDetailsForm.tsx(3 hunks)src/components/Common/ActionList.tsx(2 hunks)src/components/Global/PostSignupActionManager/index.tsx(1 hunks)src/components/Global/PostSignupActionManager/post-signup-action.consts.ts(1 hunks)src/components/Kyc/index.tsx(2 hunks)src/components/Profile/views/IdentityVerification.view.tsx(2 hunks)src/components/Request/views/ReqFulfillBankFlowManager.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T09:08:19.266Z
Learnt from: kushagrasarathe
PR: peanutprotocol/peanut-ui#1112
File: src/components/Claim/Link/views/BankFlowManager.view.tsx:336-343
Timestamp: 2025-08-20T09:08:19.266Z
Learning: In the KYC flow implementation, `setJustCompletedKyc` must be called after `await fetchUser()` in the `handleKycSuccess` callback. Setting `justCompletedKyc` before fetching the user would cause a re-fetching loop because `handleKycSuccess` is set in a useEffect inside the KYC hook, which would cause the UI flow to get stuck in one view.
Applied to files:
src/components/Kyc/index.tsx
🧬 Code graph analysis (3)
src/components/Kyc/index.tsx (1)
src/utils/general.utils.ts (1)
saveRedirectUrl(1214-1218)
src/components/Global/PostSignupActionManager/post-signup-action.consts.ts (1)
src/components/Global/Icons/Icon.tsx (1)
IconName(64-124)
src/app/[...recipient]/client.tsx (2)
src/context/RequestFulfillmentFlowContext.tsx (1)
useRequestFulfillmentFlow(102-108)src/hooks/useDetermineBankRequestType.ts (1)
useDetermineBankRequestType(18-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (7)
src/components/Kyc/index.tsx (1)
27-27: LGTM: step tracking hook integration is consistent.
useClaimLink().addParamStep('bank')aligns with the flow in ActionList and keeps URL-driven navigation cohesive.src/components/Common/ActionList.tsx (2)
98-101: LGTM: bank-step URL param added before guest verification.This matches the KYC modal behavior and keeps bank flow tracking consistent.
187-193: Confirm redirect behavior contract on GuestVerificationModal.Passing
redirectToVerificationlooks right; please verify the component defaults and that closing the modal still works without redirection in other call sites.src/components/Request/views/ReqFulfillBankFlowManager.tsx (1)
117-121: KYC success callback ordering — verify against KYC hook expectations.Per our past learning, if you use a
justCompletedKycflag, set it only afterawait fetchUser(). Confirm whether such a flag exists in this flow; if yes, ensure ordering to avoid refetch loops.src/components/Profile/views/IdentityVerification.view.tsx (1)
49-51: LGTM: update payload now uses fullName directly.src/components/Global/PostSignupActionManager/post-signup-action.consts.ts (1)
18-22: Copy and icon LGTM.Title/description are consistent with the verification flow, CTA is clear, and
'check'is a valid IconName.src/components/Global/PostSignupActionManager/index.tsx (1)
62-62: Confirm BaseModal enforces preventClose and ensure a visible exit when preventClose=true
- ActionModal accepts and forwards preventClose and toggles the close-button via hideModalCloseButton (src/components/Global/ActionModal/index.tsx).
- Repo search did not locate the Modal implementation imported from '@/components/Global/Modal' — cannot verify that preventClose blocks backdrop clicks and Escape or how classButtonClose is applied.
- Verify BaseModal implements: (1) when preventClose=true, backdrop click and Escape are disabled; (2) when preventClose=true and there is only one CTA, an accessible close control remains (visible "X" or a secondary "Not now" CTA) to avoid trapping users.
src/components/Global/PostSignupActionManager/post-signup-action.consts.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsx (2)
244-254: initialUserDetails still uses first/last name — migrate to fullName to match form and payloadThis will otherwise break TS types and prefill logic. Remove the split and provide
fullNameinstead.Apply:
- const [firstName, ...lastNameParts] = (user?.user.fullName ?? '').split(' ') - const lastName = lastNameParts.join(' ') - const initialUserDetails: Partial<UserDetailsFormData> = useMemo( - () => ({ - firstName: user?.user.fullName ? firstName : '', - lastName: user?.user.fullName ? lastName : '', - email: user?.user.email ?? '', - }), - [user?.user.fullName, user?.user.email, firstName, lastName] + () => ({ + fullName: user?.user.fullName ?? '', + email: user?.user.email ?? '', + }), + [user?.user.fullName, user?.user.email] )
281-287: Fix initialUserDetails shape — pass fullName (not firstName/lastName)UserDetailsForm expects UserDetailsFormData = { fullName, email } but src/app/(mobile-ui)/add-money/[country]/bank/page.tsx builds initialUserDetails as { firstName, lastName, email }, so fullName will be undefined. Change the useMemo to return { fullName: user?.user.fullName ?? '', email: user?.user.email ?? '' } and update the same pattern in src/components/Request/views/ReqFulfillBankFlowManager.tsx and src/components/Profile/views/IdentityVerification.view.tsx.
🧹 Nitpick comments (5)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsx (5)
183-191: Use the thrown error message instead of relying on possibly stale hook error
onrampErrormay not reflect the current failure; prefer the caught error with a fallback.- } catch (error) { - setShowWarningModal(false) - if (onrampError) { - setError({ - showError: true, - errorMessage: onrampError, - }) - } - } + } catch (err: any) { + const message = + (err && err.message) || onrampError || 'Could not start onramp. Please try again.' + setError({ showError: true, errorMessage: message }) + }
123-131: Add currency symbol to minimum-deposit validation errorImproves UX clarity across countries. Remember to update deps to avoid stale symbol.
- if (amount && amount < minimumAmount) { - setError({ showError: true, errorMessage: `Minimum deposit is ${minimumAmount}.` }) + if (amount && amount < minimumAmount) { + const currencyCode = selectedCountry ? getCurrencyConfig(selectedCountry.id, 'onramp').currency : undefined + const symbol = currencyCode ? getCurrencySymbol(currencyCode) : '' + setError({ showError: true, errorMessage: `Minimum deposit is ${symbol}${minimumAmount}.` }) return false }- [setError, minimumAmount] + [setError, minimumAmount, selectedCountry?.id]
333-342: Avoid double getCurrencyConfig callsMinor perf/readability: compute once before JSX or memoize.
Example (outside JSX):
const onrampCurrency = useMemo(() => { if (!selectedCountry) return undefined const cfg = getCurrencyConfig(selectedCountry.id, 'onramp') return { code: cfg.currency, symbol: getCurrencySymbol(cfg.currency), price: 1 } }, [selectedCountry?.id])Then pass
currency={onrampCurrency}.
36-38: Remove unused risk-acceptance state, or wire it into the modal
isRiskAcceptedis set/reset but never read nor passed toOnrampConfirmationModal.- const [isRiskAccepted, setIsRiskAccepted] = useState<boolean>(false) ... - setIsRiskAccepted(false) ... - setIsRiskAccepted(false)Also applies to: 195-197
102-109: Duplicate “advance to inputAmount” pathsBoth the KYC-status effect and
onKycSuccessmove toinputAmount. Consolidate to a single path to avoid double state churn and edge races.Would you prefer to rely solely on websocket status (and drop
onKycSuccess), or keeponKycSuccessand remove the status watcher forstep === 'kyc'?Also applies to: 203-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T14:42:54.411Z
Learnt from: Zishan-7
PR: peanutprotocol/peanut-ui#1094
File: src/utils/withdraw.utils.ts:181-191
Timestamp: 2025-08-14T14:42:54.411Z
Learning: The countryCodeMap in src/components/AddMoney/consts/index.ts uses uppercase 3-letter country codes as keys (like 'AUT', 'BEL', 'CZE') that map to 2-letter country codes, requiring input normalization to uppercase for proper lookups.
Applied to files:
src/app/(mobile-ui)/add-money/[country]/bank/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
src/app/(mobile-ui)/add-money/[country]/bank/page.tsx (1)
216-221: Switch to unified fullName in updateUserById — good changePayload now aligns with the new single-name model.
Hugo0
left a comment
Hugo0
left a comment
There was a problem hiding this comment.
minor PR. has one unnecessary log line. I don't see anything critical
src/components/Global/PostSignupActionManager/post-signup-action.consts.ts
Show resolved
Hide resolved
2 participants
This PR also contributes to TASK-14541
Rest of the changes are related to redirect flow- so cant paste SS