Skip to content

[TASK-16106] fix: values getting switched in TokenAmountInput #1395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Zishan-7 merged 1 commit into from
Nov 5, 2025
Merged

[TASK-16106] fix: values getting switched in TokenAmountInput #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/app/(mobile-ui)/withdraw/manteca/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export default function MantecaWithdrawFlow() {
const queryClient = useQueryClient()
const { isUserBridgeKycApproved } = useKycStatus()
const { hasPendingTransactions } = usePendingTransactions()

const swapCurrency = searchParams.get('swap-currency') ?? 'false'
Copy link
Contributor

@Hugo0 Hugo0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment on what this var is for would be good (swap currency is what; the currency we're swapping into, or from, or what?)

// Get method and country from URL parameters
const selectedMethodType = searchParams.get('method') // mercadopago, pix, bank-transfer, etc.
const countryFromUrl = searchParams.get('country') // argentina, brazil, etc.
Expand Down Expand Up @@ -400,7 +400,7 @@ export default function MantecaWithdrawFlow() {
walletBalance={
balance ? formatAmount(formatUnits(balance, PEANUT_WALLET_TOKEN_DECIMALS)) : undefined
}
isInitialInputUsd
isInitialInputUsd={swapCurrency !== 'true'}
/>
<Button
variant="purple"
Expand Down
21 changes: 20 additions & 1 deletion src/components/Global/TokenAmountInput/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { PEANUT_WALLET_TOKEN_DECIMALS, STABLE_COINS } from '@/constants'
import { tokenSelectorContext } from '@/context'
import { formatTokenAmount, formatCurrency } from '@/utils'
import { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'
import { useRouter, useSearchParams } from 'next/navigation'
import Icon from '../Icon'
import { twMerge } from 'tailwind-merge'
import { Icon as IconComponent } from '@/components/Global/Icons/Icon'
Expand Down Expand Up @@ -58,6 +59,8 @@ const TokenAmountInput = ({
defaultSliderSuggestedAmount,
}: TokenAmountInputProps) => {
const { selectedTokenData } = useContext(tokenSelectorContext)
const router = useRouter()
const searchParams = useSearchParams()
const inputRef = useRef<HTMLInputElement>(null)
const inputType = useMemo(() => (window.innerWidth < 640 ? 'text' : 'number'), [])
const [isFocused, setIsFocused] = useState(false)
Expand Down Expand Up @@ -173,7 +176,13 @@ const TokenAmountInput = ({
// to change when we change the display mode or the value (we already call
// onchange on the input change so dont add those dependencies here!)
useEffect(() => {
onChange(displayValue, isInputUsd)
if (!isInitialInputUsd) {
const value = tokenValue ? Number(tokenValue) : 0
const formattedValue = (value * (currency?.price ?? 1)).toFixed(2)
onChange(formattedValue, isInputUsd)
} else {
onChange(displayValue, isInputUsd)
}
}, [selectedTokenData?.price]) // Seriously, this is ok
Comment on lines +179 to 186
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Missing dependencies and unclear conditional logic.

This useEffect has several issues:

  1. Stale closures: The effect uses tokenValue, currency?.price, isInputUsd, displayValue, isInitialInputUsd, and onChange, but none are in the dependency array. This will cause the effect to capture stale values from the initial render.

  2. Semantic confusion: isInitialInputUsd controls the initial display mode, but using it to conditionally handle price changes is unclear. When a token's price changes, the conversion should be recalculated regardless of the initial mode.

  3. Missing onChange dependency: onChange is a memoized callback that can change, but it's not in the dependencies.

Consider either:

  • Adding all used variables to the dependency array and verifying the effect logic still works correctly
  • Or restructuring this logic to avoid dependencies on runtime state if only price-change handling is intended

The comment "Seriously, this is ok" no longer applies given the added complexity.

🤖 Prompt for AI Agents 
In src/components/Global/TokenAmountInput/index.tsx around lines 179 to 186, the
effect depends on multiple values but only lists selectedTokenData?.price:
update the effect so it doesn't capture stale closures by including all
referenced variables in the dependency array (tokenValue, currency?.price,
isInputUsd, displayValue, isInitialInputUsd, onChange and
selectedTokenData?.price), and simplify the conditional so price changes always
trigger a recalculation of the converted value (compute formattedValue from
current tokenValue and currency.price and call onChange with the appropriate
isInputUsd flag) rather than gating on isInitialInputUsd; alternatively, if the
intent is to only respond to price changes, isolate that behaviour in a
dedicated effect that only reads current tokenValue, currency.price and onChange
(with them in deps) to recompute and emit the converted value.

Copy link
Contributor

@Hugo0 Hugo0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coderabbit very upset at u

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hugo0 😅 I know, I know! But hear me out – this effect is doing some gymnastics that might bite later:

The useEffect is using tokenValue, currency?.price, isInputUsd, displayValue, and isInitialInputUsd but only depends on selectedTokenData?.price. This means those other values will be "frozen" from when the component first rendered, not their current values when the price changes.

The specific risk: When the token price updates, the effect will:

  • In the !isInitialInputUsd branch: use a potentially stale tokenValue and currency?.price
  • In the else branch: use a potentially stale displayValue

This might work fine initially but could cause subtle bugs when users interact with the component before/after price updates.

Quick fix options:

  1. Add all used variables to the deps: [selectedTokenData?.price, tokenValue, currency?.price, isInputUsd, displayValue, isInitialInputUsd, onChange] (but verify this doesn't cause infinite loops)
  2. Or restructure to only react to price changes without reading other state

Happy to dig deeper if you'd like! What behavior were you seeing that led to this fix? 🤔

🧠 Learnings used 
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1332
File: src/components/Global/TokenAmountInput/index.tsx:141-150
Timestamp: 2025-10-24T13:44:39.473Z
Learning: In the `TokenAmountInput` component (`src/components/Global/TokenAmountInput/index.tsx`), the slider feature (controlled by `showSlider` prop) is only shown for USD input mode. When the slider is used with `maxAmount`, the `selectedAmount` is computed in USD and `isInputUsd` is always `true`, so the conversion in `onChange` handles it correctly.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 495
File: src/components/Global/TokenAmountInput/index.tsx:23-30
Timestamp: 2024-10-29T12:19:41.968Z
Learning: In the `TokenAmountInput` component (`src/components/Global/TokenAmountInput/index.tsx`), when the 'Max' button is clicked, we intentionally set the input denomination to 'TOKEN' because we are setting the value as token.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.

Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1104
File: src/components/Payment/PaymentForm/index.tsx:596-600
Timestamp: 2025-08-22T07:25:59.304Z
Learning: The `TokenAmountInput` component in `src/components/Global/TokenAmountInput/` always returns decimal strings (e.g., "1,234.56"), not base units. When passing these values to external APIs like Daimo's `toUnits` prop, simply stripping commas with `.replace(/,/g, '')` is sufficient.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 495
File: src/components/Create/Link/Input.view.tsx:244-248
Timestamp: 2024-10-29T12:20:47.207Z
Learning: In the `TokenAmountInput` component within `src/components/Global/TokenAmountInput/index.tsx`, when `balance` is undefined, the `maxValue` prop should be set to an empty string `''`.

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.

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.

Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1368
File: src/components/Common/ActionList.tsx:109-111
Timestamp: 2025-10-29T11:27:59.248Z
Learning: In `src/components/Common/ActionList.tsx`, the `balance` from `useWallet()` hook is always in USDC (as a formatted string), making it directly comparable to USD amounts without conversion. The comparison `Number(balance) >= amountInUsd` is intentional and correct.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 424
File: src/components/Global/TokenSelector/TokenSelector.tsx:197-211
Timestamp: 2024-10-11T01:14:15.489Z
Learning: In `src/components/Global/TokenSelector/TokenSelector.tsx`, when the calculation within functions like `byChainAndText` is not computationally expensive, it's acceptable to avoid using `useCallback` for memoization.

Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 422
File: src/components/Request/Pay/Pay.tsx:103-111
Timestamp: 2024-10-07T13:42:00.443Z
Learning: When the token price cannot be fetched in `src/components/Request/Pay/Pay.tsx` within the `PayRequestLink` component, set `tokenPriceData.price` to 0 to ensure the UI remains functional. Since Squid uses their own price engine for x-chain fulfillment transactions, this approach will not affect the transaction computation.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 551
File: src/components/Request/Create/Views/Initial.view.tsx:151-156
Timestamp: 2024-12-02T17:19:18.532Z
Learning: In the `InitialView` component at `src/components/Request/Create/Views/Initial.view.tsx`, when setting the default chain and token in the `useEffect` triggered by `isPeanutWallet`, it's acceptable to omit the setters from the dependency array and not include additional error handling for invalid defaults.

Learnt from: Hugo0
Repo: peanutprotocol/peanut-ui PR: 422
File: src/components/Request/Pay/Pay.tsx:103-111
Timestamp: 2024-10-08T20:13:42.967Z
Learning: When the token price cannot be fetched in `src/components/Request/Pay/Pay.tsx` within the `PayRequestLink` component, set `tokenPriceData.price` to 0 to ensure the UI remains functional. Since Squid uses their own price engine for x-chain fulfillment transactions, this approach will not affect the transaction computation.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 564
File: src/components/Request/Pay/Views/Initial.view.tsx:430-430
Timestamp: 2024-12-11T10:13:22.806Z
Learning: In the React TypeScript file `src/components/Request/Pay/Views/Initial.view.tsx`, when reviewing the `InitialView` component, do not flag potential issues with using non-null assertion `!` on the `slippagePercentage` variable, as handling undefined values in this context is considered out of scope.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 535
File: src/components/Claim/Claim.tsx:142-146
Timestamp: 2024-11-18T21:36:11.486Z
Learning: In `src/components/Claim/Claim.tsx`, external calls like token price fetching and cross-chain details retrieval are already encapsulated within existing `try...catch` blocks, so additional error handling may be unnecessary.

Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1185
File: src/components/Claim/useClaimLink.tsx:14-0
Timestamp: 2025-09-05T07:31:11.396Z
Learning: In the peanut-ui codebase, `window.history.replaceState` is preferred over `router.replace` when immediate/synchronous URL parameter updates are required, as `router.replace` is asynchronous and doesn't guarantee instant URL changes that subsequent code can rely on. This pattern is used consistently across usePaymentInitiator.ts, Confirm.payment.view.tsx, and useClaimLink.tsx.

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.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 495
File: src/components/Create/useCreateLink.tsx:647-657
Timestamp: 2024-10-29T16:06:38.812Z
Learning: In the React code for `useCreateLink` in `src/components/Create/useCreateLink.tsx`, the `switchNetwork` function used within `useCallback` hooks is stable and does not need to be included in the dependency arrays.

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: 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.

Learnt from: jjramirezn
Repo: peanutprotocol/peanut-ui PR: 919
File: src/components/Withdraw/views/Initial.withdraw.view.tsx:87-87
Timestamp: 2025-06-18T19:56:55.443Z
Learning: In withdraw flows for Peanut Wallet, the PeanutActionDetailsCard should always display "USDC" as the token symbol because it shows the amount being withdrawn from the Peanut Wallet (which holds USDC), regardless of the destination token/chain selected by the user. The TokenSelector is used for choosing the withdrawal destination, not the source display.


useEffect(() => {
Expand Down Expand Up @@ -346,6 +355,16 @@ const TokenAmountInput = ({
setAlternativeDisplayValue(currentValue)
}
setIsInputUsd(!isInputUsd)

// Toggle swap-currency parameter in URL
Copy link
Contributor

@Hugo0 Hugo0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: putting state in url

const params = new URLSearchParams(searchParams.toString())
const currentSwapValue = params.get('swap-currency')
if (currentSwapValue === 'true') {
params.set('swap-currency', 'false')
} else {
params.set('swap-currency', 'true')
}
router.replace(`?${params.toString()}`, { scroll: false })
}}
>
<Icon name={'switch'} className="ml-5 rotate-90 cursor-pointer" width={32} height={32} />
Expand Down
Loading