Skip to content

fix(withdraw): stop method selection bouncing back to saved accounts#2432

Merged
kushagrasarathe merged 3 commits into
devfrom
fix/withdraw-crypto-method-bounce
Jul 16, 2026
Merged

fix(withdraw): stop method selection bouncing back to saved accounts#2432
kushagrasarathe merged 3 commits into
devfrom
fix/withdraw-crypto-method-bounce

Conversation

@kushagrasarathe

@kushagrasarathe kushagrasarathe commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Problem

On /withdraw with saved bank accounts: tapping "Select new method" → "Crypto" landed the user back on the saved-accounts list instead of the amount input. Separately, an open country list would snap back to saved accounts on its own after a few seconds (sometimes immediately).

Fix

  • AddWithdrawRouterView.onCryptoClick (withdraw flow) now only sets the crypto method in WithdrawFlowContext — same contract as bank-country selection — instead of also pushing /withdraw/crypto pre-amount. The withdraw page flips to the amount step and navigates to /withdraw/crypto after Continue.
  • The default-view effect in AddWithdrawRouterView now latches once per mount (ref guard). User refetches still sync the saved-accounts list but no longer force the view back to saved accounts.
  • New regression tests in src/components/AddWithdraw/__tests__/AddWithdrawRouterView.test.tsx — both fail on the unfixed code, pass with the fix.

Risks / breaking changes

  • FE-only, scoped to the withdraw method-selection screen. The add flow shares the component; its crypto path (confirmation modal) and country navigation are untouched.
  • No cross-repo impact.

QA

  1. On staging/preview with a user that has saved bank accounts, go to /withdraw.
  2. Tap "Select new method" → country list stays put (previously reverted within ~4s when a rail poll / focus refetch fired).
  3. Tap "Crypto" → amount input appears; enter an amount ≥ $1 → Continue → lands on /withdraw/crypto initial view.
  4. Complete a crypto withdrawal → "Back to home" still goes home (the race da943d68d originally fixed remains fixed — reset still happens on unmount, which now only ever fires post-amount or on genuine exit).

Postmortem

Regression window: landed on dev 2026-07-14 via PR #2393 (mobile-release merge). Breaking commit: da943d68d "fix(withdraw): 'Back to home' no longer bounces to the withdraw screen" (authored 2026-07-02 on the mobile-release branch).

What broke (symptom 1 — Crypto click):
da943d68d moved resetWithdrawFlow() from the success view's onComplete to the crypto page's unmount cleanup, to fix a real race where completing a withdrawal bounced "Back to home" to /withdraw. But /withdraw/crypto has a render guard: no amountToWithdrawrouter.push('/withdraw'). The method-selection screen had always navigated to /withdraw/crypto on the Crypto click before an amount exists, relying on that guard bouncing back with selectedMethod still intact in context (→ amount input step). After da943d68d, the bounce also unmounts the crypto page, whose new cleanup wipes the entire flow context (selectedMethod, showAllWithdrawMethods) — so the user lands on the saved-accounts list.

Chain: Crypto click → set method + push /withdraw/crypto → no-amount guard pushes back → unmount cleanup resets flow → no method selected + showAll=false → saved accounts.

What broke (symptom 2 — country list reverting on its own):
Long-latent defect, not from da943d68d. The default-view effect in AddWithdrawRouterView has user in its deps and unconditionally called setShowAllWithdrawMethods(false) whenever saved accounts exist. Every /users/me fetch dispatches a fresh user object into redux (userActions.setUser(payload) — no identity preservation), and useUserQuery refetches on window focus, on mount, and on a 4s interval while any rail is pending (useUserAutoRefresh). Each refetch re-fired the effect and yanked the open country list back to saved accounts — "after a few seconds" = the poll tick; "sometimes immediately" = an in-flight refetch resolving right after the click.

Why it wasn't caught: the pre-amount /withdraw/crypto entry path (method selection, as opposed to send-flow ?method=crypto which skips the screen) had no test; withdraw-states.test.tsx mocks out AddWithdrawRouterView entirely. da943d68d was verified against the success-flow race it fixed, not against the pre-amount entry that shares the same guard + cleanup.

Prevention: the new tests pin both behaviors: crypto selection must not navigate pre-amount, and user-object identity churn must not override an explicit view choice.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented the add/withdraw screen from unexpectedly switching views when account information refreshes.
    • Improved the withdrawal crypto selection flow so users continue to the next step without unnecessary navigation.
  • Tests

    • Added coverage for crypto method selection and view stability during account refreshes.

Two interacting defects made 'Select new method' unusable when saved
bank accounts exist:

1. Crypto click navigated to /withdraw/crypto before an amount was set.
   That page's no-amount guard bounces straight back to /withdraw, and
   since da943d6 (mobile-release, merged to dev via #2393) its unmount
   cleanup calls resetWithdrawFlow(), wiping selectedMethod and
   showAllWithdrawMethods — landing the user on the saved-accounts list
   instead of the amount input. Set the method in context only (same
   contract as bank-country selection); the withdraw page navigates to
   /withdraw/crypto after Continue, with the amount set.

2. The default-view effect re-ran on every user refetch (window focus,
   4s pending-rail poll — each dispatches a fresh user object into
   redux) and unconditionally forced the saved-accounts view, yanking
   an open country list back after a few seconds. Latch the default
   view once per mount; refetches now only sync the accounts list.
@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
peanut-wallet Ready Ready Preview, Comment Jul 16, 2026 3:06pm

Request Review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c84aa71-c4b6-4b62-97e7-11d0a3f2df71

📥 Commits

Reviewing files that changed from the base of the PR and between 776995c and b8e9bc4.

📒 Files selected for processing (2)
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx
  • src/components/AddWithdraw/__tests__/AddWithdrawRouterView.test.tsx

📝 Walkthrough

Walkthrough

AddWithdrawRouterView now preserves its initial default view across user refetches and delegates withdraw crypto selection to flow context instead of navigating immediately. New tests cover saved-account rendering, context updates, navigation suppression, and view stability.

Changes

Withdraw router behavior

Layer / File(s) Summary
Router initialization and method selection
src/components/AddWithdraw/AddWithdrawRouterView.tsx, src/components/AddWithdraw/__tests__/AddWithdrawRouterView.test.tsx
Default-view selection is latched after user resolution, while withdraw crypto clicks update selectedMethod through context without immediate router navigation. Tests cover initial saved-account rendering, crypto selection, navigation suppression, and stability after user refetches.

Estimated code review effort: 2 (Simple) | ~15 minutes

Suggested labels: enhancement

Suggested reviewers: hugo0

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/withdraw-crypto-method-bounce

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Comment @coderabbitai help to get the list of available commands.

@kushagrasarathe

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code-analysis diff

Painscore total: 6204.89 → 6205.21 (+0.32)
Findings: 0 net (+7 new, -7 resolved)

🆕 New findings (7)

  • critical complexity — src/components/AddWithdraw/AddWithdrawRouterView.tsx — CC 87, MI 57.58, SLOC 228
  • medium high-mdd — src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — AddWithdrawRouterView: MDD 96.4 (uses across many lines from declarations)
  • medium high-dlt — src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — AddWithdrawRouterView: DLT 37 (calls 37 distinct functions — high context load)
  • medium high-mdd — src/components/AddWithdraw/AddWithdrawRouterView.tsx:137 — : MDD 20.3 (uses across many lines from declarations)
  • medium method-complexity — src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — CC 19 SLOC 72
  • low high-mdd — src/components/AddWithdraw/AddWithdrawRouterView.tsx:323 — : MDD 16.5 (uses across many lines from declarations)
  • low structural-dup — components/AddWithdraw/AddWithdrawRouterView.tsx:103 — 8 duplicate lines / 53 tokens with hooks/useSavedAccounts.tsx:22

✅ Resolved (7)

  • src/components/AddWithdraw/AddWithdrawRouterView.tsx — CC 87, MI 57.96, SLOC 230
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — AddWithdrawRouterView: MDD 105.0 (uses across many lines from declarations)
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — AddWithdrawRouterView: DLT 35 (calls 35 distinct functions — high context load)
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx:58 — CC 20 SLOC 71
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx:129 — : MDD 20.3 (uses across many lines from declarations)
  • src/components/AddWithdraw/AddWithdrawRouterView.tsx:315 — : MDD 16.5 (uses across many lines from declarations)
  • components/AddWithdraw/AddWithdrawRouterView.tsx:99 — 8 duplicate lines / 53 tokens with hooks/useSavedAccounts.tsx:22

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🧪 UI test report — ✅ all green

Suites

  • unit: 1983 ran, 0 failed, 0 skipped, 32.6s

📊 Coverage (unit)

metric %
statements 59.5%
branches 43.1%
functions 48.5%
lines 59.8%
⏱ 10 slowest test cases
time test
3.5s src/components/Card/share-asset/__tests__/shareAssetLayout.test.ts › never places two stickers in heavy overlap (broad seed sweep)
1.0s src/utils/__tests__/demo-api.test.ts › isDemoMode() is false when not running under Capacitor
0.4s src/components/Card/share-asset/__tests__/shareAssetLayout.test.ts › every sticker stays within canvas at any count
0.3s src/app/actions/__tests__/api-headers.test.ts › should include Content-Type in validateInviteCode
0.3s src/app/actions/__tests__/api-headers-extended.test.ts › should not include apiKey in validateInviteCode body
0.2s src/components/Card/share-asset/__tests__/shareAssetLayout.test.ts › keeps stickers off the username pill (final pass respects the keep-out)
0.2s src/utils/__tests__/url.utils.test.ts › uses the public BASE_URL in Capacitor, not the localhost WebView origin
0.1s src/components/Global/GeneralRecipientInput/__tests__/GeneralRecipientInput.test.tsx › should handle valid 9-digit US account
0.1s src/utils/__tests__/demo-balance.test.ts › keeps a spent-down balance across a cold start within the TTL
0.1s src/utils/__tests__/demo-balance.test.ts › debits and floors at zero
📍 Inline annotations are in the **Unit test report** check above. Coverage artifact: `coverage-unit`. Generated by `.github/workflows/tests.yml`.

… add-flow default

code-review round: the withdraw crypto click now goes through
handleMethodSelected (one code path for 'a withdraw method was selected'
— analytics payload + method shape can't drift), the add flow skips its
localstorage re-read + state churn once the default view is latched, and
the regression tests run against the real WithdrawFlowContextProvider
instead of a hand-rolled context copy that could drift from it.
@kushagrasarathe

Copy link
Copy Markdown
Contributor Author

Deep review pass (multi-agent /code-review high) — outcomes

Applied in b8e9bc4:

  • crypto click now delegates to handleMethodSelected — one code path for "withdraw method selected", so the analytics payload / method shape can't silently drift.
  • add flow skips the localStorage re-read + state churn once the default view is latched (was running every 4s during pending-rail polling).
  • regression tests now run against the real WithdrawFlowContextProvider instead of a hand-rolled context copy.

Investigated and NOT applied (with reasons):

  • Stale selectedMethod on abandoned session (re-entering /withdraw shows the amount step for a previously chosen method; can leak across into the send flow): real, but pre-existing — the bank path on dev behaves identically today, and this matches pre-Merge mobile release (native iOS/Android) into main #2393 behavior. Every quick fix shape breaks a legit flow: the native app (AddWithdrawCountriesList) intentionally does setSelectedMethod(...) → router.push('/withdraw') with no amount, i.e. "method set + no amount at mount" is a valid state, not a reliable staleness signal. Proper fix is structural (scope the provider to the withdraw segment, or move method/step to URL state per the repo's URL-as-state rule) → follow-up.
  • useSavedAccounts dedup (the saved-account type filter here is a hand copy of the hook's canonical list — this drift class already bit Manteca once): worth doing, but the hook sources user from authContext while this component uses the redux store — swapping sources belongs in its own reviewed change → follow-up.
  • View mode should be URL state (?view=all via nuqs): agreed per repo convention; would make the bounce class impossible by construction and remove the ref latch. Structural → same follow-up as above.
  • Crypto page's no-amount redirect guard + reset-on-unmount remains a trap for any future pre-amount navigation into /withdraw/crypto; contained for now (deep links/refreshes start with fresh context) → note for the withdraw-flow refactor.

@kushagrasarathe
kushagrasarathe marked this pull request as ready for review July 16, 2026 15:26
@kushagrasarathe
kushagrasarathe merged commit 1851669 into dev Jul 16, 2026
15 of 17 checks passed
@kushagrasarathe

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant