Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughDerives a memoized paymentProcessor from QR type, passes it into the updated useQrKycGate hook (adding a SIMPLEFI fast-path), sets a default currency for SimpleFi USER_SPECIFIED flows, and expands SimpleFi QR regexes and tests to support both pagar.simplefi.tech and pay.simplefi.tech. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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). (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useQrKycGate.ts (1)
33-71: Prevent stale Manteca requests from overriding the SimpleFi bypass
When a Manteca QR triggers thegetBridgeCustomerCountryfetch and the user immediately scans a SimpleFi QR, the old Manteca promise can still resolve/reject afterwards and overwrite the newPROCEED_TO_PAYstate (e.g. thecatchpath setsREQUIRES_IDENTITY_VERIFICATION). That breaks the SimpleFi KYC bypass intermittently. Please guard the async branch so only the latestpaymentProcessorrun is allowed to mutate state—for example, track the current processor in a ref and bail out before setting state if it no longer matches.-import { useCallback, useState, useEffect } from 'react' +import { useCallback, useState, useEffect, useRef } from 'react' @@ -export function useQrKycGate(paymentProcessor?: 'MANTECA' | 'SIMPLEFI' | null): QrKycGateResult { +export function useQrKycGate(paymentProcessor?: 'MANTECA' | 'SIMPLEFI' | null): QrKycGateResult { const { user } = useAuth() const [kycGateState, setKycGateState] = useState<QrKycState>(QrKycState.LOADING) + const latestPaymentProcessorRef = useRef(paymentProcessor) + + useEffect(() => { + latestPaymentProcessorRef.current = paymentProcessor + }, [paymentProcessor]) const determineKycGateState = useCallback(async () => { + const runProcessor = paymentProcessor // SimpleFi payments do not require KYC - allow payment immediately - if (paymentProcessor === 'SIMPLEFI') { + if (runProcessor === 'SIMPLEFI') { setKycGateState(QrKycState.PROCEED_TO_PAY) return } @@ - if (currentUser.bridgeKycStatus === 'approved' && currentUser.bridgeCustomerId) { + if (currentUser.bridgeKycStatus === 'approved' && currentUser.bridgeCustomerId) { try { const { countryCode } = await getBridgeCustomerCountry(currentUser.bridgeCustomerId) + if (latestPaymentProcessorRef.current !== runProcessor) { + return + } // if (countryCode && countryCode.toUpperCase() === 'AR') { if (false) { setKycGateState(QrKycState.REQUIRES_MANTECA_KYC_FOR_ARG_BRIDGE_USER) } else { setKycGateState(QrKycState.PROCEED_TO_PAY) } } catch { + if (latestPaymentProcessorRef.current !== runProcessor) { + return + } // fail to require identity verification to avoid blocking pay due to rare outages setKycGateState(QrKycState.REQUIRES_IDENTITY_VERIFICATION) } return } }, [user?.user, paymentProcessor])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(mobile-ui)/qr-pay/page.tsx(1 hunks)src/components/Global/DirectSendQR/__tests__/recognizeQr.test.ts(5 hunks)src/components/Global/DirectSendQR/utils.ts(1 hunks)src/hooks/useQrKycGate.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 1014
File: src/components/Claim/Link/Initial.view.tsx:413-413
Timestamp: 2025-07-24T13:26:10.290Z
Learning: In the peanut-ui repository, the change from `${SQUID_API_URL}/route` to `${SQUID_API_URL}/v2/route` in src/components/Claim/Link/Initial.view.tsx was a typo fix, not an API migration, as the codebase was already using Squid API v2.
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 1190
File: src/app/(mobile-ui)/qr-pay/page.tsx:156-176
Timestamp: 2025-09-08T03:13:09.111Z
Learning: In the peanut-ui mobile app, the `/qr-pay` route is only accessed through the DirectSendQR component which always includes the qrCode parameter in the URL when redirecting users to the QR pay page.
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 1190
File: src/app/(mobile-ui)/qr-pay/page.tsx:156-176
Timestamp: 2025-09-08T03:13:09.111Z
Learning: In the peanut-ui mobile app, the `/qr-pay` route is only accessed through the DirectSendQR component which always includes the qrCode parameter in the URL when redirecting users to the QR pay page after scanning MERCADO_PAGO or PIX QR codes.
Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 942
File: src/components/AddMoney/consts/index.ts:2151-2162
Timestamp: 2025-06-30T10:44:08.048Z
Learning: Hugo0 often agrees with refactoring suggestions but defers implementation due to time constraints, preferring to track improvements as follow-up issues when they're part of larger architectural changes.
📚 Learning: 2025-08-11T10:35:02.715Z
Learnt from: kushagrasarathe
Repo: peanutprotocol/peanut-ui PR: 1078
File: src/hooks/useKycFlow.ts:129-141
Timestamp: 2025-08-11T10:35:02.715Z
Learning: In the KYC flow implementation in `src/hooks/useKycFlow.ts`, when Terms of Service (ToS) is accepted, there will always be a KYC link available in the `apiResponse`. The system ensures this invariant, so defensive checks for missing KYC links after ToS acceptance are unnecessary.
Applied to files:
src/hooks/useQrKycGate.ts
📚 Learning: 2025-08-20T09:08:19.266Z
Learnt from: kushagrasarathe
Repo: peanutprotocol/peanut-ui PR: 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/hooks/useQrKycGate.ts
📚 Learning: 2024-10-23T09:38:04.446Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 469
File: src/app/request/pay/page.tsx:32-64
Timestamp: 2024-10-23T09:38:04.446Z
Learning: Within `src/app/request/pay/page.tsx`, extracting the `getBaseUrl` function does not add significant readability, and the host URL construction code is expected to change soon.
Applied to files:
src/components/Global/DirectSendQR/utils.tssrc/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2024-10-07T15:25:45.170Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-10-07T15:25:45.170Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2025-09-18T09:30:42.901Z
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1230
File: src/app/(mobile-ui)/withdraw/page.tsx:92-97
Timestamp: 2025-09-18T09:30:42.901Z
Learning: In src/app/(mobile-ui)/withdraw/page.tsx, the useEffect that calls setShowAllWithdrawMethods(true) when amountFromContext exists is intentionally designed to run only on component mount (empty dependency array), not when amountFromContext changes. This is the correct behavior for the withdraw flow where showing all methods should only happen on initial load when an amount is already present.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2025-10-02T15:23:01.513Z
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1266
File: src/app/(mobile-ui)/withdraw/[country]/bank/page.tsx:46-57
Timestamp: 2025-10-02T15:23:01.513Z
Learning: In the withdraw flow at src/app/(mobile-ui)/withdraw/[country]/bank/page.tsx, the points calculation query intentionally uses crypto.randomUUID() in the queryKey dependency array to bypass React Query caching, ensuring fresh points estimates on every render. This is the intended behavior.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2025-09-08T03:13:09.111Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 1190
File: src/app/(mobile-ui)/qr-pay/page.tsx:156-176
Timestamp: 2025-09-08T03:13:09.111Z
Learning: In the peanut-ui mobile app, the `/qr-pay` route is only accessed through the DirectSendQR component which always includes the qrCode parameter in the URL when redirecting users to the QR pay page.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2024-10-07T15:28:25.280Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-10-07T15:28:25.280Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2024-10-22T18:10:56.955Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 469
File: src/app/request/pay/page.tsx:25-25
Timestamp: 2024-10-22T18:10:56.955Z
Learning: In the `src/app/request/pay/page.tsx` file, the `PreviewType` enum values are strings, so when adding `previewType` to `URLSearchParams`, there's no need to convert them to strings.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2025-08-22T07:28:32.281Z
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1104
File: src/components/Payment/PaymentForm/index.tsx:522-545
Timestamp: 2025-08-22T07:28:32.281Z
Learning: In `src/components/Payment/PaymentForm/index.tsx`, the `handleCompleteDaimoPayment` function is only for updating payment status in the backend after a successful Daimo payment. Payment success/failure is handled by Daimo itself, so try/catch error handling and error display are not needed for backend sync failures - users shouldn't see errors if payment succeeded but database update failed.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2025-09-08T03:13:09.111Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 1190
File: src/app/(mobile-ui)/qr-pay/page.tsx:156-176
Timestamp: 2025-09-08T03:13:09.111Z
Learning: In the peanut-ui mobile app, the `/qr-pay` route is only accessed through the DirectSendQR component which always includes the qrCode parameter in the URL when redirecting users to the QR pay page after scanning MERCADO_PAGO or PIX QR codes.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
📚 Learning: 2024-10-22T18:11:36.864Z
Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 469
File: src/app/request/pay/page.tsx:32-49
Timestamp: 2024-10-22T18:11:36.864Z
Learning: In `src/app/request/pay/page.tsx`, the `id` parameter is accessed via `searchParams.id` in the `generateMetadata` function.
Applied to files:
src/app/(mobile-ui)/qr-pay/page.tsx
🧬 Code graph analysis (3)
src/hooks/useQrKycGate.ts (1)
src/context/authContext.tsx (1)
useAuth(192-198)
src/components/Global/DirectSendQR/__tests__/recognizeQr.test.ts (1)
src/components/Global/DirectSendQR/utils.ts (1)
recognizeQr(113-135)
src/app/(mobile-ui)/qr-pay/page.tsx (1)
src/hooks/useQrKycGate.ts (1)
useQrKycGate(29-96)
🪛 GitHub Actions: Tests
src/hooks/useQrKycGate.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
⏰ 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
Contributor
Author
|
@coderabbitai review deeply |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 110506 @Hugo0, I'll conduct a comprehensive deep review of this PR. Let me analyze the security, correctness, and edge cases. 🔍 Deep Review Findings1. SECURITY CONCERN: Regex Patterns Allow Empty Merchant SlugsThe SIMPLEFI_USER_SPECIFIED_REGEX =
/^(?:https?:\/\/)?(?:www\.)?(?:pagar|pay)\.simplefi\.tech\/(?<merchantSlug>[^\/\?]*)(?:\/)?(?:\?.*)?$/Issue:
Impact: In return simpleFiQrData.merchantSlug.replace(/-/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase())This will return an empty string for display, which is poor UX. Recommendation: Change regex to require at least one character: SIMPLEFI_USER_SPECIFIED_REGEX =
/^(?:https?:\/\/)?(?:www\.)?(?:pagar|pay)\.simplefi\.tech\/(?<merchantSlug>[^\/\?]+)(?:\/)?(?:\?.*)?$/
// ^ plus instead of asterisk2. SECURITY: KYC Bypass Logic Needs ValidationThe KYC bypass in if (paymentProcessor === 'SIMPLEFI') {
setKycGateState(QrKycState.PROCEED_TO_PAY)
return
}✅ Good: The bypass happens at the hook level before any user checks.
const qrType = searchParams.get('type')
const paymentProcessor: PaymentProcessor | null = useMemo(() => {
switch (qrType) {
case EQrType.SIMPLEFI_STATIC:
// ...Security Question: Can users manipulate the URL to bypass KYC? Let me trace the flow: 🏁 Script executed: Length of output: 147 🏁 Script executed: Length of output: 9273 🏁 Script executed: Length of output: 3110 🏁 Script executed: Length of output: 6559 🏁 Script executed: Length of output: 735 🏁 Script executed: Length of output: 497 🏁 Script executed: Length of output: 2026 @Hugo0, I've completed a comprehensive deep review. Here are my findings: 🚨 MEDIUM PRIORITY: Regex Allows Empty/Whitespace Merchant IDsYour regexes allow empty and whitespace-only values in critical fields: // Current implementation
SIMPLEFI_USER_SPECIFIED_REGEX = /(?<merchantSlug>[^\/\?]*)/ // * allows ZERO or more
SIMPLEFI_DYNAMIC_REGEX = /(?<merchantId>[^\/]*)/ // * allows ZERO or more
// Edge cases that PASS validation:
"https://pay.simplefi.tech/ /123" → merchantId = " " (space)
"https://pay.simplefi.tech//123" → merchantId = "" (empty)
"https://pay.simplefi.tech/" → merchantSlug = "" (empty)Impact on UX: Line 432 in return simpleFiQrData.merchantSlug.replace(/-/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase())This displays an empty merchant name when merchantSlug is empty or whitespace-only. Recommendation: Require at least one character: export const SIMPLEFI_STATIC_REGEX =
/^(?:https?:\/\/)?(?:www\.)?(?:pagar|pay)\.simplefi\.tech\/(?<merchantSlug>[^\/]+)(?:\/static|\/?\?.*static\=true.*)/
// ^^^ plus instead of asterisk
export const SIMPLEFI_USER_SPECIFIED_REGEX =
/^(?:https?:\/\/)?(?:www\.)?(?:pagar|pay)\.simplefi\.tech\/(?<merchantSlug>[^\/\?]+)(?:\/)?(?:\?.*)?$/
// ^^^
export const SIMPLEFI_DYNAMIC_REGEX =
/^(?:https?:\/\/)?(?:www\.)?(?:pagar|pay)\.simplefi\.tech\/(?<merchantId>[^\/]+)\/(?:payment\/)?(?<paymentId>[^\/\?]+)(?:\/)?(?:\?.*)?$/
// ^^^ ^^^✅ SECURITY VALIDATED: KYC Bypass is SafeI traced the entire flow and confirmed the KYC bypass for SimpleFi is secure: Trust Boundary Analysis:
Conclusion: Even if a user manipulates the URL parameter 🟡 Test Coverage GapsYour tests don't cover these important scenarios:
Recommendation: Add tests for: describe('SIMPLEFI edge cases', () => {
it('should NOT recognize URLs with empty merchant slugs', () => {
expect(recognizeQr('https://pay.simplefi.tech/')).toBeNull()
expect(recognizeQr('https://pay.simplefi.tech/ ')).toBeNull()
})
it('should handle special characters safely', () => {
const xssSlug = 'https://pay.simplefi.tech/<script>alert(1)</script>'
// Should either reject or properly escape
})
})✅ Code Quality: What's Working Well
🔍 Minor Issues1. Inconsistent Error Handling for SimpleFi USER_SPECIFIEDIn 2. Potential Race Condition in Retry LogicLines 819-831: The retry logic for SimpleFi uses 3. Missing Merchant Name ValidationLine 432 transforms merchant slug but doesn't validate the result. Add validation: const merchantName = simpleFiQrData.merchantSlug
.replace(/-/g, ' ')
.replace(/\b\w/g, (c) => c.toUpperCase())
.trim()
return merchantName || 'SimpleFi Merchant' // Fallback for empty names📋 Summary
Overall Assessment: The PR is functionally sound with good backward compatibility. The main concerns are UX issues from empty merchant IDs and test coverage gaps. The KYC bypass is secure due to backend validation. Recommendation: Fix the regex to require at least one character in merchant IDs before merging, or add validation in the merchant name display logic to handle empty values gracefully. Would you like me to create an issue to track the regex improvements and test coverage enhancements? 🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support SimpleFi URL changes & Fix KYC requirement
Problem
SimpleFi changed their URLs:
pagar.simplefi.tech→pay.simplefi.tech/merchant/payment/123→/merchant/123(removed "payment" from path)SimpleFi payments incorrectly required KYC:
useQrKycGatehook was applying identity verification checks to all QR payment typesSolution
URL Support Changes:
pagar.simplefi.tech(legacy) andpay.simplefi.tech(new)/merchant/payment/123(old) and/merchant/123(new)KYC Fix:
useQrKycGatehook to acceptpaymentProcessorparameterpaymentProcessor === 'SIMPLEFI'Changes
peanut-ui/src/components/Global/DirectSendQR/utils.ts: Updated 3 SimpleFi regex patternspeanut-ui/src/components/Global/DirectSendQR/__tests__/recognizeQr.test.ts: Added 27 new test casespeanut-ui/src/hooks/useQrKycGate.ts: Added payment processor awarenesspeanut-ui/src/app/(mobile-ui)/qr-pay/page.tsx: Pass payment processor to KYC gatepeanut-api-ts/src/depositWatchService.ts: Fixed null check forlog.args.frompeanut-api-ts/src/requestPaymentValidator.ts: Added debug log for ongoing Squid transactionsRisks
Low Risk - Fully Backward Compatible:
pagar.simplefi.tech,/payment/format) continue to workTesting
pagarandpaysubdomains workNote
Adds support for pay.simplefi.tech (incl. new dynamic format) and makes KYC gating apply only to MANTECA, not SimpleFi.
SIMPLEFI_*regexes to support bothpagar.simplefi.techandpay.simplefi.tech, and handle dynamic URLs with/without/payment/.useQrKycGate(paymentProcessor)processor-aware; bypass KYC whenpaymentProcessor === 'SIMPLEFI'.app/(mobile-ui)/qr-pay/page.tsx, derivepaymentProcessorfromqrTypeand pass touseQrKycGate.ARSforSIMPLEFI_USER_SPECIFIEDflows.recognizeQr.test.tswith cases forpay.simplefi.tech, new dynamic format, and precedence rules.Written by Cursor Bugbot for commit 45ee052. This will update automatically on new commits. Configure here.