fix(add-money): remove the duplicate method selection, not the method screen (TASK-20033)#2476
Conversation
The add-money flow asked "Bank or Crypto" twice — once on a standalone method screen, then again on the per-country screen after picking a country. Customers reported this as buggy/confusing (TASK-20033). Collapse the redundant first screen: /add-money now lands directly on the country list (with a crypto shortcut + the offramp-migrate entry), and the country-aware per-country screen remains the single place the method is chosen. The per-country screen already knows each country's real methods and coming-soon states, so no country regresses.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesAdd Money country routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CountryList
participant AddMoneyPage
participant Router
CountryList->>AddMoneyPage: select country
AddMoneyPage->>Router: navigate to provider or country route
Router-->>AddMoneyPage: update country query
AddMoneyPage->>AddMoneyPage: reset onramp state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code-analysis diffPainscore total: 6233.75 → 6235.79 (+2.04) 🆕 New findings (6)
✅ Resolved (5)
📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`(mobile-ui)/add-money/page.tsx:
- Around line 41-43: Update the useEffect in the add-money page to depend on
countryFromQuery and resetOnrampFlow, so it reruns when the query country or
reset callback changes and clears stale onramp state when countryFromQuery is
absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98ee814e-b478-4405-9fb7-a4dcee0a0b57
📒 Files selected for processing (4)
src/app/(mobile-ui)/add-money/__tests__/add-money-states.test.tsxsrc/app/(mobile-ui)/add-money/page.tsxsrc/components/AddMoney/views/AddMoneyMethodSelection.view.tsxsrc/components/Invites/campaign-maps.ts
💤 Files with no reviewable changes (1)
- src/components/AddMoney/views/AddMoneyMethodSelection.view.tsx
…parity
- useEffect now depends on countryFromQuery + (stable) resetOnrampFlow, so
stale onramp state is cleared on native back-nav to the root, not only on
mount (CodeRabbit + code-review). Also clears the exhaustive-deps warning.
- drop the unreachable countryFromQuery branch in handleBack (the ?country
sub-views early-return their own NavHeader).
- fire DEPOSIT_METHOD_SELECTED{method_type:'crypto'} on network select so
root crypto deposits match the bank event in the deposit-method funnel.
- AddWithdrawCountriesList add-flow back-nav → '/add-money' (drop the now-dead
?method=bank param).
- cover the offramp-badge landing card (present with badge, absent without).
Switch from Option A (which deleted the Bank/Crypto method screen and left an
ambiguous country list — users couldn't tell bank was an option) to Option B:
- Keep the up-front Bank/Crypto method screen (bank is clearly visible).
- After the user picks Bank + a country, go STRAIGHT to that country's deposit
screen instead of showing a second "From Bank / From Crypto" list:
- AR/BR -> /add-money/<country>/manteca (Manteca surfaces Pix/Mercado Pago)
- other bank-supported countries -> /add-money/<country>/bank
Every add method for a given country converges on one of these two screens
anyway, so the per-country list was pure repetition where bank is live.
- Coming-soon countries (bank not enabled) keep the per-country screen — there
it's still useful: it shows the "soon" bank state and the crypto fallback.
Net effect: the user clicks "bank" once, up front; the duplicate second
selection is gone. This reverts the Option A page/view/test churn — the diff is
now just handleCountryClick routing (+ effect-deps cleanup) and its tests.
kushagrasarathe
changed the title
innolope-dev
left a comment
innolope-dev
left a comment
There was a problem hiding this comment.
Diff looks solid — the Manteca/Bridge/coming-soon routing is correct and covered by tests. Checked COUNTRY_SPECIFIC_METHODS: no country currently has two live bank options, so skipping straight to bank/manteca once "Bank" is chosen doesn't hide a real choice anywhere.
One thing worth a fix: the PR description's QA step 2 ("Tap a bank-enabled country (e.g. Germany) → per-country methods → bank flow") is now stale relative to this diff — Germany (and other Bridge-supported countries) now route straight to /add-money/germany/bank, skipping the per-country method screen entirely, per the diff's own test (selecting a Bridge-supported country goes straight to the bank deposit). Worth updating the description so it matches actual behavior for the next reader.
Heads up: this touches the same two files (page.tsx, add-money-states.test.tsx) as my open #2447 — no conflict yet since neither's merged, but whichever lands second will need a quick rebase.
Also, the eslint check failure looks like stale-cache noise rather than a real issue — I linted the [country]/bank/page.tsx errors it's flagging directly against this PR's head commit and got 0 errors, 1 unrelated warning. Wouldn't block on it.
Approving — logic and tests look right.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
The add-money flow asked users to choose "Bank or Crypto" twice — once on a standalone method screen, then again on the per-country screen after they'd already picked a country (TASK-20033).
Approach: keep the up-front method screen, remove the redundant second one. The user picks Bank / Crypto once (bank is clearly visible), and after choosing Bank + a country we go straight to that country's deposit screen instead of showing a second "From Bank / From Crypto" list:
/add-money/<country>/manteca(the Manteca screen surfaces Pix / Mercado Pago itself)/add-money/<country>/bankEvery add-method for a given country already converged on one of those two deposit screens, so the per-country list was pure repetition wherever bank is live.
Why not the first attempt (Option A)
An earlier commit deleted the method screen and made
/add-moneyland directly on the country list. It removed the wrong screen — users could no longer tell that a bank option existed (the country list gave no method context). Reverted; this PR keeps the method screen and instead collapses the second one.Changes
add-money/page.tsx—handleCountryClickroutes bank-supported countries straight to their deposit screen (Manteca/Bridge), and keeps the per-country screen only for coming-soon countries. Also fixed the mount-onlyuseEffectdeps (clears stale onramp state on back-nav to the root;resetOnrampFlowis a stableuseCallback)./manteca, Bridge →/bank, coming-soon → per-country screen).Diff is intentionally tiny — the method screen, the offramp card, and the per-country component are all unchanged from
dev.Risk
Low, FE-only, no backend/contract change. Blast radius = the country-tap handler on the add-money bank path. Coming-soon countries are unchanged (still see the per-country screen). Native (
?country=/view=) routing preserved viarewriteMethodPath.QA (sandbox)
/add-money→ Bank / Crypto method screen (bank clearly visible)./add-money/crypto?network=….Summary by CodeRabbit
New Features
Bug Fixes