fix(withdraw): Continue button can never silently die (no-throw + correct Manteca routing)#2224
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reached
More reviews will be available in 41 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughTwo independent bug fixes: ChangesderiveGate ready-first priority reordering
WithdrawPage Continue dead-button fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/capability-gate.ts (1)
156-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign non-ready branches to per-operation status semantics.
deriveGatenow evaluatesreadyusingoperationStatus(rail, op), but blocked/requires-info/pending still userail.status. This can return the wrong gate whenoperations[op]differs from top-level status.Suggested fix
- const blocked = candidates.find((rail) => rail.status === 'blocked') + const blocked = candidates.find((rail) => operationStatus(rail, op) === 'blocked') @@ - const requiresInfoRails = candidates.filter((rail) => rail.status === 'requires-info') + const requiresInfoRails = candidates.filter((rail) => operationStatus(rail, op) === 'requires-info') @@ - const hasPending = candidates.some((rail) => rail.status === 'pending') + const hasPending = candidates.some((rail) => operationStatus(rail, op) === 'pending')Also applies to: 173-174, 202-203
🤖 Prompt for 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. In `@src/utils/capability-gate.ts` around lines 156 - 157, The deriveGate function has inconsistent status evaluation across different branches. The ready branch correctly uses operationStatus(rail, op) to evaluate per-operation status, but the blocked, requires-info, and pending branches still use rail.status which reflects only top-level status and can return incorrect gates when operations[op] differs from the top-level status. Update all three affected locations (lines 156-157 for the blocked check, lines 173-174 for the requires-info check, and lines 202-203 for the pending check) to replace rail.status with operationStatus(rail, op) to align with the per-operation status semantics used for the ready evaluation.src/app/(mobile-ui)/withdraw/__tests__/withdraw-states.test.tsx (1)
247-249:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock property mismatch:
balancevsspendableBalance.The default mock returns
{ balance: parseUnits('100', 6) }, but the component destructuresspendableBalance(line 84 ofpage.tsx). The GROUP 6 tests correctly override this withspendableBalance, but the existing tests in groups 1–5 may be inadvertently relying onspendableBalancebeingundefined.Consider aligning the default mock with the actual property name:
Proposed fix
mockUseWallet.mockReturnValue({ - balance: parseUnits('100', 6), + spendableBalance: parseUnits('100', 6), })🤖 Prompt for 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. In `@src/app/`(mobile-ui)/withdraw/__tests__/withdraw-states.test.tsx around lines 247 - 249, The mockUseWallet.mockReturnValue call is providing a `balance` property, but the component destructures `spendableBalance` instead. Update the mockUseWallet.mockReturnValue object to use `spendableBalance` as the property name instead of `balance` to align the mock with the actual property name expected by the component.
🤖 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.
Outside diff comments:
In `@src/app/`(mobile-ui)/withdraw/__tests__/withdraw-states.test.tsx:
- Around line 247-249: The mockUseWallet.mockReturnValue call is providing a
`balance` property, but the component destructures `spendableBalance` instead.
Update the mockUseWallet.mockReturnValue object to use `spendableBalance` as the
property name instead of `balance` to align the mock with the actual property
name expected by the component.
In `@src/utils/capability-gate.ts`:
- Around line 156-157: The deriveGate function has inconsistent status
evaluation across different branches. The ready branch correctly uses
operationStatus(rail, op) to evaluate per-operation status, but the blocked,
requires-info, and pending branches still use rail.status which reflects only
top-level status and can return incorrect gates when operations[op] differs from
the top-level status. Update all three affected locations (lines 156-157 for the
blocked check, lines 173-174 for the requires-info check, and lines 202-203 for
the pending check) to replace rail.status with operationStatus(rail, op) to
align with the per-operation status semantics used for the ready evaluation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c9ad7b4-00fc-462c-83b8-499b3029a467
📒 Files selected for processing (4)
src/app/(mobile-ui)/withdraw/__tests__/withdraw-states.test.tsxsrc/app/(mobile-ui)/withdraw/page.tsxsrc/utils/capability-gate.test.tssrc/utils/capability-gate.ts
Hugo0
force-pushed
the
hotfix/withdraw-continue-no-throw
branch
from
4a99002 to
2d63aa1
Compare
Contributor
Code-analysis diffPainscore total: 5793.76 → 5794.08 (+0.32) 🆕 New findings (6)
✅ Resolved (6)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
handleAmountContinue threw 'Failed to get country from bank account' inside
the onClick when getCountryFromAccount returned undefined. A synchronous throw
in a click handler aborts the router transition with zero UI feedback — the
button just goes dead ('press Continue, nothing happens'). Surfaced as a
customer report + Sentry incomplete-app-router-transaction (6 users/14d).
Three changes make the handler correct-by-construction:
- Route Manteca (AR/BR) accounts by method type BEFORE the generic saved-bank
branch. Manteca accounts also set selectedBankAccount, so they previously
fell into that branch and either mis-routed to the Bridge bank page or threw.
- Replace the throw with setError + console.error so an unresolved country is a
recoverable, visible error, not a dead button.
- Add a terminal else that sets an error, so no selected-method shape can ever
leave the handler having done nothing.
The real country-resolution fix is the API counterpart (Manteca flat metadata
shape); this is the defense-in-depth that guarantees the symptom can't recur.
Adds the first tests for the no-country path and Manteca routing — the harness
previously mocked getCountryFromAccount to always succeed, so this path was
entirely uncovered.
…t-clean Avoid adding a no-require-imports eslint error: drive the bridge.utils mock through a hoisted mock fn (matching the file's mockUseWallet idiom) instead of require()-ing the mocked module inside the test.
CodeRabbit nit: the component destructures spendableBalance; the harness default mocked the wrong key (balance), leaving maxDecimalAmount=0 for existing tests.
Hugo0
force-pushed
the
hotfix/withdraw-continue-no-throw
branch
from
2d63aa1 to
62e67bd
Compare
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.
What
Makes the withdraw "Continue" handler (
handleAmountContinue) correct-by-construction so it can never silently die:selectedBankAccount, so they used to fall into that branch and either mis-route to the Bridge bank page or throw.throw new Error('Failed to get country from bank account')withsetError+console.error. A synchronous throw inside an onClick aborts the router transition with zero UI feedback — the button just goes dead.elsethat sets an error, so noselectedMethodshape can leave the handler having done nothing.Why (root cause)
Customer report (Miranda): withdraw "press Continue, nothing happens." It was a dead button, not a hang:
getCountryFromAccount(selectedBankAccount)returnedundefined, and the handler threw inside the click. Surfaced in Sentry asincomplete-app-router-transaction— 6 users / 14 days.The proximate data cause (empty projected country for flat-shape Manteca accounts) is fixed at the source in the API companion PR (peanutprotocol/peanut-api-ts#1021). This PR is the defense-in-depth: even a genuinely-unresolvable country now produces a visible, recoverable error instead of a dead control.
Bug class
A background audit found this is one instance of a broader class (handlers that
throwor silently no-op → dead button). Notably a second HIGH: the "Exchange or Wallet" card in the direct Send flow (PaymentMethodActionList) is also a dead button whenSendInputViewomitsonPayWithExternalWallet. Tracked separately.Test
Adds the first tests for the no-country path (asserts no throw, no navigation,
setErrorshown) and Manteca routing. The harness previously mockedgetCountryFromAccountto always succeed, so this path was entirely uncovered — that test gap is why the throw shipped. Suite: 17 pass / 1 pre-existing skip.