Conversation
🦋 Changeset detectedLatest commit: 2741dce The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFetches KYC status and gates add-funds flows; adds KYC initiation hooks and UI loading/disable states; unifies KYC result typing, removes persona navigation side-effects, invalidates user/country on KYC completion, and adds i18n and a changeset for a patch release. Changes
Sequence DiagramsequenceDiagram
participant User
participant AddFunds as AddFunds Component
participant KYCQuery as KYC Status Query
participant BeginKYC as useBeginKYC Hook
participant Persona as persona.startKYC()
participant Cache as Query Cache
participant Ramp as Ramp Providers Query
User->>AddFunds: Open add-funds / select option
AddFunds->>KYCQuery: Read ["kyc","status"]
alt KYC approved
AddFunds->>AddFunds: Navigate to chosen flow (fiat/crypto)
else KYC not approved
AddFunds->>BeginKYC: Call mutate() / mutateAsync()
BeginKYC->>Persona: startKYC()
Persona->>Persona: External KYC flow (complete / cancel / error)
Persona->>Cache: Invalidate ["kyc","status"]
Persona->>Cache: Invalidate ["user","country"] on complete
Persona-->>BeginKYC: Resolve with KYCResult
BeginKYC-->>AddFunds: Promise settles
AddFunds->>Ramp: Invalidate / refetch ramp providers (if applicable)
AddFunds->>AddFunds: Proceed to flow unless cancelled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Prevent Tests Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/getting-started/GettingStarted.tsx (1)
152-157: 🧹 Nitpick | 🔵 TrivialClarify the navigation condition for explicitness.
The basic KYC flow rejects (rather than resolves) on error, so
result?.status !== "cancel"effectively meansresult?.status === "complete". However, making this explicit improves readability and guards against future changes to the error handling pattern.💡 Suggested clarification
beginKYC .mutateAsync() .then((result) => { - if (result?.status !== "cancel") router.replace("/(main)/(home)"); + if (!result || result.status === "complete") router.replace("/(main)/(home)"); }) .catch(() => {}); // eslint-disable-line `@typescript-eslint/no-empty-function` -- error handled by useBeginKYCsrc/components/add-funds/AddFunds.tsx (1)
209-213:⚠️ Potential issue | 🟡 MinorConsider showing a loading state while
countryCodeis unresolved.When
countryCodeis falsy (empty string or undefined), neither the skeleton nor the providers content renders, leaving a blank content area. This can occur during the initialcountryCodefetch or on direct navigation to the fiat URL.You could destructure
isLoadingfrom thecountryCodequery and show a skeleton while it resolves:- const { data: countryCode } = useQuery({ + const { data: countryCode, isLoading: isCountryLoading } = useQuery({Then update the fiat skeleton condition:
- {type === "fiat" && countryCode && isPending && ( + {type === "fiat" && (isCountryLoading || (countryCode && isPending)) && (
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b9b1a08-1801-4366-98bc-f6d2748ac4a0
📒 Files selected for processing (10)
.changeset/stale-teams-say.mdcspell.jsonsrc/components/add-funds/AddFunds.tsxsrc/components/add-funds/AddFundsOption.tsxsrc/components/getting-started/GettingStarted.tsxsrc/i18n/es-AR.jsonsrc/i18n/es.jsonsrc/i18n/pt.jsonsrc/utils/persona.tssrc/utils/useBeginKYC.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cef13507bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| title={t("Bank transfers")} | ||
| subtitle={t("From a bank account")} | ||
| disabled={!hasFiat} | ||
| disabled={(isKYCApproved && !hasFiat) || beginKYC.isPending} |
There was a problem hiding this comment.
The new disabled condition enables Bank transfers for users who are not KYC-approved while hasFiat is still undefined (providers query still loading). In that state, tapping immediately starts KYC before we know whether fiat is actually available for the user’s country, so unsupported users can be sent through verification and still end up with no fiat option. This is a regression from the previous behavior where the CTA stayed disabled until hasFiat resolved.
Useful? React with 👍 / 👎.
| const { data: method } = useQuery<AuthMethod>({ queryKey: ["method"] }); | ||
| const { data: kycStatus } = useQuery<KYCStatus>({ queryKey: ["kyc", "status"] }); | ||
| const beginKYC = useBeginKYC(); | ||
| const isKYCApproved = kycStatus?.code === "ok" || kycStatus?.code === "legacy kyc"; |
There was a problem hiding this comment.
🟡 Inconsistent isKYCApproved pattern — missing "code" in guard used everywhere else
At src/components/add-funds/AddFunds.tsx:45, isKYCApproved is computed as kycStatus?.code === "ok" || kycStatus?.code === "legacy kyc", skipping the "code" in kycStatus narrowing guard. Every other usage in the codebase (10+ sites) follows the pattern kycStatus && "code" in kycStatus && (kycStatus.code === "ok" || ...) — including line 165 in the same file. This violates the AGENTS.md consistency rule: "consistency is more important than personal preference. adhere to the established patterns in the codebase."
All other usages follow the established pattern
src/components/card/Card.tsx:94-96src/components/home/Home.tsx:130-131src/components/getting-started/GettingStarted.tsx:31-36src/components/add-funds/AddFunds.tsx:165(same file!)src/utils/useBeginKYC.ts:24src/components/home/card-upgrade/VerifyIdentity.tsx:30
| const isKYCApproved = kycStatus?.code === "ok" || kycStatus?.code === "legacy kyc"; | |
| const isKYCApproved = Boolean(kycStatus && "code" in kycStatus && (kycStatus.code === "ok" || kycStatus.code === "legacy kyc")); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| beginKYC | ||
| .mutateAsync() | ||
| .then(async (result) => { | ||
| if (result?.status === "cancel") return; | ||
| const status = await queryClient.fetchQuery<KYCStatus>({ | ||
| queryKey: ["kyc", "status"], | ||
| staleTime: 0, | ||
| }); | ||
| if ("code" in status && (status.code === "ok" || status.code === "legacy kyc")) { | ||
| await queryClient.invalidateQueries({ queryKey: ["ramp", "providers"] }); | ||
| router.push({ pathname: "/add-funds", params: { type: "fiat" } }); | ||
| } | ||
| }) | ||
| .catch(() => {}); // eslint-disable-line @typescript-eslint/no-empty-function -- error handled by useBeginKYC |
There was a problem hiding this comment.
🚩 Silent failure when KYC webhook processing is delayed
In AddFunds.tsx:157-170, after KYC completes, the code fetches KYC status and only navigates to fiat if the status is "ok" or "legacy kyc". If the server-side webhook hasn't been processed yet (race between persona completion and server webhook), fetchQuery could return a non-ok status (which throws APIError from getKYCStatus), caught silently by .catch(() => {}) at line 170. The user would see no feedback — no navigation and no error toast. The useBeginKYC onError handler only covers errors from mutationFn, not from the .then() chain. This is an unlikely edge case but could cause user confusion.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }} | ||
| /> | ||
| {hasFiat !== false && ( | ||
| {hasFiat !== false && chain.id !== base.id && ( |
There was a problem hiding this comment.
🚩 Bank transfers option hidden on base chain is a new restriction
Line 145 adds chain.id !== base.id to hide the bank transfers option entirely on base chain. Previously, the condition was only hasFiat !== false. This means base chain users will never see the bank transfers option regardless of whether fiat providers are available. This appears intentional (the same pattern is applied to the KYC info alert at line 178), but it's a meaningful behavioral change worth confirming is deliberate.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit
New Features
Improvements
Localization
Chores