Conversation
WalkthroughReplaces single aggregated stable balance with per-chain stable-balance aggregation and a summed total; raises USD minimum to $2; adds per-chain native-token gas-affordability checks and dynamic messages; updates Buy UI/MAX behavior and BuyButton liquidity short-circuit; exports native-token detection and symbol helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Buy as Buy.tsx
participant Portfolio as Portfolio Utils
participant Chain as Blockchain Utils
participant Price as Pricing/FX
User->>Buy: Enter USD amount / click MAX
Buy->>Portfolio: getStableCurrencyBalanceOnEachChain(walletPortfolioData)
Portfolio-->>Buy: map(chainId -> stableBalance)
Buy->>Buy: compute sumOfStableBalance, find chain with maxStableCoinBalance
Buy->>Portfolio: convertPortfolioAPIResponseToToken(maxChainId)
Portfolio-->>Buy: token list for chain
Buy->>Chain: isNativeToken(token.address)
alt native token found
Buy->>Price: convert native token balance -> USD
Price-->>Buy: nativeUSD
alt nativeUSD < 1
Buy->>Buy: set minGasFee & minGasFeeChain
else
Buy->>Buy: clear gas-related state
end
end
alt sumOfStableBalance < 2 USD or input < 2 USD
Buy-->>User: show minimumStableBalance / below-min messages
else
Buy-->>User: proceed with buy flow
end
sequenceDiagram
autonumber
actor User
participant Btn as BuyButton.tsx
User->>Btn: Render BuyButton
Btn->>Btn: isDisabled()
alt notEnoughLiquidity == true
Btn-->>User: Disabled (short-circuit)
else
Btn-->>User: Disabled/Enabled per other combined conditions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Deploying x with
|
| Latest commit: |
5260d42
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c3ba6de.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3692-warning-messages.x-e62.pages.dev |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
649-689: Include the new warning flags inshowError.
showErrorstill ignoresminimumStableBalanceandminGasFee, so the UI returnsnullbefore it ever reaches the new messages; users never see the deposit prompts. Fold those booleans into the guard so the warning actually renders.Recommended change:
const showError = belowMinimumAmount || insufficientWalletBalance || + minimumStableBalance || + minGasFee || (notEnoughLiquidity && token) || (!isLoading && expressIntentResponse && expressIntentResponse.bids?.length === 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/pulse/components/Buy/Buy.tsx(10 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(2 hunks)src/apps/pulse/utils/blockchain.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (2)
isNativeToken(17-18)getNativeTokenSymbol(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (1)
src/apps/pulse/components/Buy/BuyButton.tsx (1)
107-110: Early bail-out keeps the button state honest.The new guard short-circuits on the liquidity flag, which prevents the later checks from masking it. Nicely simplifies the disable logic.
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
| ); | ||
| }; | ||
|
|
||
| const getStableCurrencyBalanceOnEachChain = () => { |
There was a problem hiding this comment.
Surely we can reduce this function using getStableCurrencyBalance no? This seems like a lot of repeated code
There was a problem hiding this comment.
I can do one thing use this as helper fn and total it when the total balance is required
There was a problem hiding this comment.
what is the difference between getStableCurrencyBalance and getStableCurrencyBalanceOnEachChain? And Why do we need both?
There was a problem hiding this comment.
I have changed it now pls check
There was a problem hiding this comment.
Ok but I do not see why this was changed since the previous getStableCurrencyBalance was also at the end getting the sum of all USD values in someone's wallet
There was a problem hiding this comment.
getStableCurrencyBalance was already doing what getStableCurrencyBalanceOnEachChain + the useEffect you added. It was doing the exact same thing
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
| setNoEnoughLiquidity(true); | ||
| return; | ||
| } | ||
| const stableBalance = getStableCurrencyBalance(); |
There was a problem hiding this comment.
Currently the Pulse app is not able to mix all stable balances together, it only checks for now each balance on each chain. So if you have 0.3 USD on chain 56 and 1.7 USD on chain 137, it will not be able to sum those two together, it would look at each USD balance separately. So look if the total stable balance is above 2 here will not work, until we implement multi chain USD.
There was a problem hiding this comment.
no it stores the USD value and for any chain the USD value is the same and that's why you are able to get total stable coin balance as one and show it to the user in that please see the bottom of input USD that's the total value of all stable coins
There was a problem hiding this comment.
I mean for pulse and intentSDK, currently it handles USD one by one not together. So in my example above, you cannot buy tokens for 2 USD, but for a max of 1.7 USD (the highest single USD balance)
There was a problem hiding this comment.
so we should check balance individually or what since on the ticket it was shown like this
There was a problem hiding this comment.
@vignesha22 in the scenario that user is inputting within the cumulative balance of USDC across all chains. User inputs a balance that is not part of a single chain. Lets say cumalative balance is $20 but $10 is on Optimism, $5 is on Arbitrum and $5 is on Base. The warning message should state: "Not enough USDC, reduce amount to $10. "
There was a problem hiding this comment.
@vignesha22 I think some extra work need to be done to do as @aldin4u said above
| ]); | ||
|
|
||
| const refreshBuyIntent = useCallback(async () => { | ||
| const stable_Balance = getStableCurrencyBalanceOnEachChain(); |
There was a problem hiding this comment.
I feel we have several stablebalance const in this file, this is repeated code.
There was a problem hiding this comment.
Done pls check latest code
| (key) => stable_Balance[Number(key)] === maxStableBalance | ||
| ) || '1' | ||
| ); | ||
| if (!walletPortfolioData) return; |
There was a problem hiding this comment.
Maybe this check needs to sit above it all.
| ); | ||
| if (!walletPortfolioData) return; | ||
| // Take the native token balance of that chain | ||
| const tokens = convertPortfolioAPIResponseToToken(walletPortfolioData.result.data); |
There was a problem hiding this comment.
I feel tokens, and nativeToken might be already showing or coded somewhere in the Pulse app, to avoid repetition here
There was a problem hiding this comment.
I checked and we don't get native balance anywhere that's why we didn't have native addresses on utils
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
676-689: Fix Fragment and formatting issues.Multiple critical issues detected:
- Line 676: Unnecessary Fragment wrapper (contains only one child)
- Lines 683-689: Multiple Prettier formatting violations
Remove the Fragment and run Prettier to resolve all formatting issues:
- return ( - <> - <div className="flex items-center justify-center"> + return ( + <div className="flex items-center justify-center">Then run Prettier to auto-format lines 683-689.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/Buy/Buy.tsx(10 hunks)src/apps/pulse/utils/blockchain.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (2)
isNativeToken(17-18)getNativeTokenSymbol(29-31)
🪛 GitHub Actions: vignesha22 has committed new code! 🚀 Checking...
src/apps/pulse/utils/blockchain.ts
[error] 31-31: Prettier formatting: insert a newline.
[error] 31-31: Prettier formatting issue detected.
src/apps/pulse/components/Buy/Buy.tsx
[error] 18-18: Prettier formatting issue. Replace ·chainNameToChainIdTokensData,·convertPortfolioAPIResponseToToken· with a properly formatted multi-line sequence.
[error] 174-174: Prettier formatting issue. Replace ⏎················contract.address.toLowerCase()·&&· with a single-line expression or proper wrapping.
[error] 184-184: Prettier formatting issue. Delete the stray newline.
[error] 194-194: Prettier formatting issue. Delete the stray line.
[error] 196-196: Prettier formatting issue. Insert a semicolon.
[error] 271-271: Naming convention violation. Variable name stable_Balance must match camelCase, PascalCase, or UPPER_CASE.
[error] 281-281: Prettier formatting issue. Replace walletPortfolioData.result.data with a multi-line wrapped form.
[error] 282-282: Prettier formatting issue. Delete stray indentation.
[error] 284-284: Prettier formatting issue. Break the arrow function formatting across lines as recommended.
[error] 285-285: Prettier formatting issue. Delete stray indentation.
[error] 287-287: Prettier formatting issue. Insert required line breaks before the next block.
[error] 676-676: Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all.
[error] 683-683: Prettier formatting issue. Insert newline formatting after JSX opening tag.
[error] 684-684: Prettier formatting issue. Insert required indentation.
[error] 685-685: Prettier formatting issue. Replace multiple spaces with proper formatting.
[error] 686-686: Prettier formatting issue. Insert required indentation.
[error] 687-687: Prettier formatting issue. Insert required indentation.
[error] 688-688: Prettier formatting issue. Insert required indentation.
[error] 689-689: Prettier formatting issue. Format JSX to satisfy prettier rules for data-testid attribute block.
[error] 676-676: Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (8)
src/apps/pulse/utils/blockchain.ts (3)
11-15: LGTM!The Set includes the standard native token addresses (zero address, EIP-1167, and Polygon's native token address) and provides efficient O(1) lookup.
17-18: LGTM!The function correctly checks native token membership with case-insensitive comparison.
20-27: LGTM!The mapping correctly associates chainIds with their native token symbols, including the updated 'POL' for Polygon (chainId 137) after the MATIC rebrand.
src/apps/pulse/components/Buy/Buy.tsx (5)
112-114: LGTM!The new state variables appropriately track minimum balance and gas fee conditions for the enhanced validation logic.
223-223: LGTM!The minimum buy amount has been correctly updated to 2 USD as per the PR objectives.
246-251: LGTM!The logic now correctly resets
minimumStableBalancetofalsewhen the balance meets the threshold, addressing the issue raised in the previous review.
662-670: LGTM!The error messages are clear and provide helpful context, including the specific native token symbol when gas fees are insufficient.
774-776: LGTM!The
notEnoughLiquidityprop correctly incorporates the newminimumStableBalanceandminGasFeeconditions to disable the buy button when these validation checks fail.
| const getStableCurrencyBalanceOnEachChain = () => { | ||
| // get the list of chainIds from STABLE_CURRENCIES | ||
| const chainIds = Array.from( | ||
| new Set(STABLE_CURRENCIES.map((currency) => currency.chainId)) | ||
| ); | ||
|
|
||
| // create a map to hold the balance for each chainId | ||
| const balanceMap: { [chainId: number]: number } = {}; | ||
| chainIds.forEach((chainId) => { | ||
| balanceMap[chainId] = 0; | ||
| }); | ||
| // calculate the balance for each chainId | ||
| walletPortfolioData?.result.data.assets | ||
| ?.filter((asset) => | ||
| asset.contracts_balances.some((contract) => | ||
| STABLE_CURRENCIES.some( | ||
| (stable) => | ||
| stable.address.toLowerCase() === | ||
| contract.address.toLowerCase() && | ||
| stable.chainId === Number(contract.chainId.split(':').at(-1)) | ||
| ) | ||
| ) | ||
| ) | ||
| .forEach((asset) => { | ||
| const stableContracts = asset.contracts_balances.filter((contract) => | ||
| STABLE_CURRENCIES.some( | ||
| (stable) => | ||
| stable.address.toLowerCase() === | ||
| contract.address.toLowerCase() && | ||
| stable.chainId === Number(contract.chainId.split(':').at(-1)) | ||
| ) | ||
| ); | ||
| stableContracts.forEach((contract) => { | ||
| const chainId = Number(contract.chainId.split(':').at(-1)); | ||
| balanceMap[chainId] += asset.price * contract.balance; | ||
| }); | ||
| }); | ||
|
|
||
| return balanceMap; | ||
| } |
There was a problem hiding this comment.
Fix formatting and consider refactoring to reduce duplication.
Multiple pipeline failures detected:
- Line 174: Improper line wrapping
- Line 184: Stray newline
- Line 194: Stray line
- Line 196: Missing semicolon
Additionally, this function duplicates significant logic from getStableCurrencyBalance (lines 124-155). As noted in a previous review comment, this could be refactored to reduce repetition.
Immediate fix: Run Prettier to resolve formatting issues.
Recommended refactor: Consider extracting shared filtering logic into a helper function that both getStableCurrencyBalance and getStableCurrencyBalanceOnEachChain can use.
🧰 Tools
🪛 GitHub Actions: vignesha22 has committed new code! 🚀 Checking...
[error] 174-174: Prettier formatting issue. Replace ⏎················contract.address.toLowerCase()·&&· with a single-line expression or proper wrapping.
[error] 184-184: Prettier formatting issue. Delete the stray newline.
[error] 194-194: Prettier formatting issue. Delete the stray line.
[error] 196-196: Prettier formatting issue. Insert a semicolon.
🤖 Prompt for AI Agents
In src/apps/pulse/components/Buy/Buy.tsx around lines 157 to 196, the
getStableCurrencyBalanceOnEachChain function has formatting errors (improper
line wrapping at ~174, stray newlines at ~184 and ~194, and a missing semicolon
at ~196) and duplicates filtering/contract-matching logic already implemented in
getStableCurrencyBalance (lines 124–155); run Prettier to fix the formatting
issues, correct the stray line breaks and add the missing semicolon, then
refactor by extracting the shared filtering/matching logic into a small helper
(e.g., getStableContractsForAsset(asset, STABLE_CURRENCIES) and/or a function to
compute chainId from contract.chainId) and call that helper from both
getStableCurrencyBalance and getStableCurrencyBalanceOnEachChain to eliminate
duplication and keep balance aggregation logic in place.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const stable_Balance = getStableCurrencyBalanceOnEachChain(); | ||
| // Take the Max value of stable currency balance across chains and also take the chainId | ||
| const maxStableBalance = Math.max(...Object.values(stable_Balance)); | ||
| const chainIdOfMaxStableBalance = Number( | ||
| Object.keys(stable_Balance).find( | ||
| (key) => stable_Balance[Number(key)] === maxStableBalance | ||
| ) || '1' | ||
| ); | ||
| if (!walletPortfolioData) return; | ||
| // Take the native token balance of that chain | ||
| const tokens = convertPortfolioAPIResponseToToken(walletPortfolioData.result.data); | ||
| const nativeToken = | ||
| tokens.find( | ||
| (t) => Number(getChainId(t.blockchain as MobulaChainNames)) === chainIdOfMaxStableBalance && isNativeToken(t.contract) | ||
| ); | ||
| if (!nativeToken) return; | ||
| const nativeTokenUSDBalance = (nativeToken.balance ?? 0) * (nativeToken.price ?? 0); | ||
| if (!nativeTokenUSDBalance || nativeTokenUSDBalance < 1) { | ||
| setMinGasFee(true); | ||
| setMinGasFeeChain(getNativeTokenSymbol(chainIdOfMaxStableBalance)); | ||
| return; | ||
| } | ||
| setMinGasFee(false); | ||
| setMinGasFeeChain(''); |
There was a problem hiding this comment.
Fix variable naming and formatting issues.
Multiple critical issues detected:
- Line 271: Variable
stable_Balanceviolates camelCase naming convention (should bestableBalance) - Lines 281-287: Multiple Prettier formatting violations
Immediate fix: Rename stable_Balance to stableBalance and run Prettier to resolve formatting issues.
Consider adding error handling: The call to convertPortfolioAPIResponseToToken at line 281 could potentially throw errors. Consider wrapping this logic in a try-catch block to prevent breaking the buy flow.
🧰 Tools
🪛 GitHub Actions: vignesha22 has committed new code! 🚀 Checking...
[error] 271-271: Naming convention violation. Variable name stable_Balance must match camelCase, PascalCase, or UPPER_CASE.
[error] 281-281: Prettier formatting issue. Replace walletPortfolioData.result.data with a multi-line wrapped form.
[error] 282-282: Prettier formatting issue. Delete stray indentation.
[error] 284-284: Prettier formatting issue. Break the arrow function formatting across lines as recommended.
[error] 285-285: Prettier formatting issue. Delete stray indentation.
[error] 287-287: Prettier formatting issue. Insert required line breaks before the next block.
🤖 Prompt for AI Agents
In src/apps/pulse/components/Buy/Buy.tsx around lines 271 to 294, rename the
variable stable_Balance to stableBalance and update all its usages (Math.max,
Object.values/keys, and chainId lookup) to use camelCase; fix
Prettier/formatting violations in the block (especially lines ~281-287) by
running the project Prettier settings to normalize indentation/spacing; and wrap
the call to convertPortfolioAPIResponseToToken and subsequent token logic in a
try-catch that logs or safely handles errors to avoid breaking the buy flow
(ensure returns or fallback behavior are preserved and adjust any early returns
accordingly).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } else if (minimumStableBalance) { | ||
| message = 'You need $2 USDC to trade, deposit USDC'; | ||
| } else if (minGasFee) { | ||
| message = `You need at least $1 of ${minGasFeeChain} to trade, deposit ${minGasFeeChain}`; |
There was a problem hiding this comment.
SHouldn't we indicate on which chain they need to deposit?
There was a problem hiding this comment.
it automatically gets the symbol of that chainId and store it in minGasFeeChain which is then displayed to the user
There was a problem hiding this comment.
Yes so it show the symbol, not the chain name. So here you are saying You need at least 1$ of ETH to trade, deposit ETH ---> but does nto say on which chain
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/apps/pulse/components/Buy/Buy.tsx (3)
125-164: Consider refactoring to reduce duplication with existing balance calculation.As noted in previous review discussions, this function shares similar logic with the existing stable currency balance calculation. While the current implementation is functional, extracting the shared filtering/contract-matching logic into a reusable helper would improve maintainability.
This refactor can be deferred if time-constrained, but would benefit the codebase long-term.
249-273: Consider adding error handling for token conversion.The call to
convertPortfolioAPIResponseToTokenat line 260 could potentially throw an error. While the early return at line 265 handles the missing native token case, wrapping the conversion in a try-catch would make the code more resilient.Example:
+ try { const tokens = convertPortfolioAPIResponseToToken(walletPortfolioData.result.data); const nativeToken = tokens.find( (t) => Number(getChainId(t.blockchain as MobulaChainNames)) === chainIdOfMaxStableBalance && isNativeToken(t.contract) ); + } catch (error) { + console.error('Failed to process native token balance:', error); + return; + }
649-649: Consider including the chain name in the gas fee warning.As noted in previous review discussions, the warning message shows which native token is needed but doesn't indicate which chain to deposit to. While the token symbol provides some context, explicitly mentioning the chain would improve user experience.
This could be addressed in a follow-up if the team determines it's needed based on user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Buy/Buy.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (2)
isNativeToken(17-18)getNativeTokenSymbol(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
267-273: Chain name missing in gas fee warning context.The warning state stores only the native token symbol (e.g., "ETH"), but when rendered at line 651, the message "You need at least $1 of ETH to trade, deposit ETH" doesn't indicate which chain. This is ambiguous since multiple chains share the same native token symbol (ETH on Ethereum mainnet, Base, Arbitrum, and Optimism; all map to "ETH").
Consider storing both the symbol and chain name, or use a more specific message format.
Example solution:
+// At the top of the component, add a helper or use existing chain name mapping +const getChainName = (chainId: number): string => { + const chainNames: Record<number, string> = { + 1: 'Ethereum', + 137: 'Polygon', + 8453: 'Base', + 42161: 'Arbitrum', + 10: 'Optimism', + 56: 'BNB Chain', + }; + return chainNames[chainId] || `Chain ${chainId}`; +}; if (!nativeTokenUSDBalance || nativeTokenUSDBalance < 1) { setMinGasFee(true); - setMinGasFeeChain(getNativeTokenSymbol(chainIdOfMaxStableBalance)); + setMinGasFeeChain(`${getNativeTokenSymbol(chainIdOfMaxStableBalance)} on ${getChainName(chainIdOfMaxStableBalance)}`); return; }Then at line 651, the message becomes: "You need at least $1 of ETH on Base to trade, deposit ETH on Base"
Based on past review comments, this issue was previously flagged by RanaBug but not fully resolved.
🧹 Nitpick comments (2)
src/apps/pulse/components/Buy/Buy.tsx (2)
128-165: Consider extracting helper for filtering stable contracts.The function is correct but has nested filtering logic that could be simplified for readability. Consider extracting a helper function to find matching stable currency contracts.
Example refactor:
const getStableCurrencyBalanceOnEachChain = () => { const chainIds = Array.from( new Set(STABLE_CURRENCIES.map((currency) => currency.chainId)) ); const balanceMap: { [chainId: number]: number } = {}; chainIds.forEach((chainId) => { balanceMap[chainId] = 0; }); // Helper to check if contract matches a stable currency const isStableContract = (contract: any) => STABLE_CURRENCIES.some( (stable) => stable.address.toLowerCase() === contract.address.toLowerCase() && stable.chainId === Number(contract.chainId.split(':').at(-1)) ); walletPortfolioData?.result.data.assets ?.filter((asset) => asset.contracts_balances.some(isStableContract) ) .forEach((asset) => { const stableContracts = asset.contracts_balances.filter(isStableContract); stableContracts.forEach((contract) => { const chainId = Number(contract.chainId.split(':').at(-1)); balanceMap[chainId] += asset.price * contract.balance; }); }); return balanceMap; };
361-361: Consider memoizing the helper function.Including
getStableCurrencyBalanceOnEachChain(defined at lines 128-165) in the dependency array causesrefreshBuyIntentto be recreated on every render, since the helper function gets a new reference each time. This could impact performance.Options to consider:
- Wrap
getStableCurrencyBalanceOnEachChaininuseCallbackwith proper dependencies- Move the function outside the component if it doesn't need component state
- Accept the current behavior if the team has determined it's acceptable for this use case
Based on learnings, the team has accepted similar trade-offs for floating-point arithmetic, so this may be an acceptable pattern for this codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/Buy/Buy.tsx(13 hunks)src/apps/pulse/utils/blockchain.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (2)
isNativeToken(17-18)getNativeTokenSymbol(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (11)
src/apps/pulse/utils/blockchain.ts (2)
11-18: LGTM!The native token address recognition is correctly implemented with case-insensitive matching. The Set data structure ensures efficient O(1) lookups.
20-31: LGTM!The chain-to-symbol mapping is accurate and includes all supported chains. The default fallback to 'ETH' is reasonable given Ethereum's prevalence.
src/apps/pulse/components/Buy/Buy.tsx (9)
18-21: LGTM!The new imports are necessary to support per-chain balance calculations and native token symbol resolution.
115-117: LGTM!The new state variables appropriately track minimum balance and gas fee requirements for the enhanced validation flow.
167-172: LGTM!The effect correctly aggregates per-chain stable balances into a single total, with appropriate dependencies.
223-227: LGTM! Previous review comment addressed.The minimum stable balance check now correctly resets
minimumStableBalanceto false when the balance recovers (line 227), fixing the issue flagged in a previous review where the state would remain true indefinitely.
643-643: LGTM! Minimum amount updated.The minimum buy amount has been correctly updated from 0.5 to 2 USD, as specified in the PR objectives.
657-674: LGTM! Warning alignment improved.The warning icon and message are now properly aligned using flexbox, addressing the alignment issue mentioned in the PR objectives.
692-692: LGTM!The wallet balance display now correctly shows the aggregated stable balance across all chains.
717-717: LGTM!The MAX button now correctly uses the total stable balance, ensuring users can utilize their full balance across all chains.
757-759: LGTM!The liquidity check now properly includes the new validation states (minimum stable balance and gas fee requirements), ensuring the BuyButton is disabled when these conditions aren't met.
| return; | ||
| } | ||
|
|
||
| if (sumOfStableBalance < 2) { |
There was a problem hiding this comment.
We are still looking here at the total sum of stable balance. However Pulse at the moment is not using the total some of USD to buy a token, it looks at all balances, and chose the balance of the USD that could be enough to buy the tokem
There was a problem hiding this comment.
okay I will take the most and check whether it has 2 USD
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/apps/pulse/components/Buy/Buy.tsx (2)
279-285: Native token absence should trigger gas-fee warning.When
convertPortfolioAPIResponseToTokendoesn't return a native token (common when the user has zero native balance), the code setsminGasFee=falseand returns. This allows the flow to proceed without a gas-fee warning, even though the wallet can't cover transaction costs.This issue was previously flagged in earlier review comments but remains unresolved.
As suggested in the previous review, apply this fix:
if (!nativeToken) { console.warn( 'Native token not found for chainId:', maxStableCoinBalance.chainId ); - setMinGasFee(false); + setMinGasFee(true); return; }
736-736: MAX button uses sum instead of max single-chain balance.The MAX button sets the USD amount to
sumOfStableBalance, which aggregates balances across all chains. Since Pulse can only execute trades using balance from a single chain, this will always trigger an "Insufficient wallet balance" error when the sum exceeds the largest single-chain balance.This issue was previously flagged in earlier review comments but remains unresolved.
As suggested in the previous review, apply this fix:
if (isMax) { - setUsdAmount(sumOfStableBalance.toFixed(2)); + setUsdAmount(maxStableCoinBalance.balance.toFixed(2)); } else {
🧹 Nitpick comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
662-670: Consider making the insufficient balance message more specific.The error messages are clear and well-structured. The native token warning at line 670 now dynamically includes both the token symbol and chain name, which addresses the previous review feedback.
However, the "Not enough USDC to buy" message at line 666 could be more helpful by indicating the maximum available amount on the single chain, as suggested in previous review discussions:
Not enough USDC, reduce amount to $${maxStableCoinBalance.balance.toFixed(2)}.Apply this diff to provide more actionable guidance:
- message = 'Not enough USDC to buy'; + message = `Not enough USDC, reduce amount to $${maxStableCoinBalance.balance.toFixed(2)}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Buy/Buy.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (3)
isNativeToken(17-18)NativeSymbols(20-27)ChainNames(29-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (8)
src/apps/pulse/components/Buy/Buy.tsx (8)
18-21: LGTM!The new imports for
convertPortfolioAPIResponseToTokenand blockchain utilities (ChainNames,isNativeToken,NativeSymbols) are properly added and necessary for the per-chain balance calculation and native token validation features.Also applies to: 39-43
119-124: LGTM!The new state variables properly support the per-chain balance tracking requirements:
minimumStableBalanceandminGasFeefor validation flagsmaxStableCoinBalanceto track the chain with the highest stable balancesumOfStableBalancefor displaying total available balanceAlso applies to: 132-132
135-172: LGTM!The
getStableCurrencyBalanceOnEachChainfunction correctly computes per-chain stable balances by:
- Creating a map of chainId to aggregated USD balance
- Filtering assets that contain stable currency contracts
- Summing
price * balancefor each stable contract grouped by chainThe logic properly handles the per-chain aggregation requirement mentioned in the PR objectives.
174-189: LGTM!The
useEffectcorrectly derives:
sumOfStableBalance: Total stable balance across all chains for display purposesmaxStableCoinBalance: The chain with the highest stable balance and its amount, used for validationThis aligns with the requirement that Pulse can only use balances from a single chain per transaction, while still displaying the total available balance to users.
216-216: LGTM!The minimum buy amount has been correctly updated from
0.5to2USD, as specified in the PR objectives.
314-318: LGTM!Correctly validates the input amount against
maxStableCoinBalance.balancerather than the sum, since Pulse can only execute trades using balance from a single chain.
711-711: LGTM!Displaying
sumOfStableBalancein the wallet balance indicator is correct—it shows users their total available funds across all chains, which is appropriate for the balance display even though individual trades are constrained to single-chain balances.
773-779: LGTM!The
notEnoughLiquidityprop correctly includes the new validation flags (minimumStableBalanceandminGasFee) to disable the buy button when minimum balance or gas fee requirements aren't met.
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
730-736: MAX should use the largest single-chain stable balance (not the sum).Pulse cannot aggregate stables across chains for a single buy. Setting MAX to
sumOfStableBalancewill frequently exceedmaxStableCoinBalance.balanceand trigger "Insufficient wallet balance" at line 310.Apply this diff:
onClick={() => { if (!isDisabled) { if (isMax) { - setUsdAmount(sumOfStableBalance.toFixed(2)); + setUsdAmount(maxStableCoinBalance.balance.toFixed(2)); } else { setUsdAmount(parseFloat(item).toFixed(2)); }
🧹 Nitpick comments (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
665-666: Consider specifying the chain for USDC deposits.The message "You need $2 USDC to trade, deposit USDC" doesn't tell users which chain to deposit on. Since
maxStableCoinBalance.chainIdidentifies the chain with the most USDC, referencing it would guide users more effectively.Example:
- } else if (minimumStableBalance) { - message = 'You need $2 USDC to trade, deposit USDC'; + } else if (minimumStableBalance) { + message = `You need $2 USDC to trade, deposit USDC on ${ChainNames[maxStableCoinBalance.chainId]}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Buy/Buy.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/Buy.tsx (3)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/utils/constants.ts (1)
getChainId(28-49)src/apps/pulse/utils/blockchain.ts (3)
isNativeToken(17-18)NativeSymbols(20-27)ChainNames(29-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (10)
src/apps/pulse/components/Buy/Buy.tsx (10)
18-21: LGTM! Clean imports for new functionality.The multi-line imports are well-formatted and bring in the necessary utilities for per-chain balance tracking and native token detection.
Also applies to: 39-43
119-124: LGTM! Well-structured state for multi-chain balance tracking.The state variables properly track per-chain maximums, aggregated totals, and new error conditions. The default
balance: 2formaxStableCoinBalancesensibly allows trading until the first wallet data fetch completes.Also applies to: 132-132
135-172: LGTM! Correctly aggregates stable balances per chain.The function properly filters stable currency contracts, matches them by address and chain ID, and computes the USD-denominated balance for each chain. This per-chain breakdown is essential for the updated liquidity checks that respect Pulse's single-chain buy constraint.
174-189: LGTM! Efficient balance aggregation on wallet data changes.The
useEffectcorrectly computes both the cross-chain sum and the maximum single-chain stable balance wheneverwalletPortfolioDataupdates. The chainId lookup with a fallback to'1'(Ethereum) is a sensible default.
216-221: LGTM! Minimum buy amount raised to $2 USD.The threshold increase aligns with the PR objectives and is consistently enforced throughout the component.
259-289: LGTM! Gas-fee checks properly guard against insufficient native balance.The logic correctly:
- Validates minimum stable balance with proper state reset (line 268 addresses the prior stuck-true concern)
- Retrieves the native token for the chain holding the maximum stable balance
- Sets
minGasFee(true)when the native token is missing or its USD value falls below $1 (lines 279-289)- Resets
minGasFee(false)when the threshold is satisfiedThis ensures users are warned before attempting a trade that would fail due to insufficient gas funds.
310-314: LGTM! Wallet balance validation respects single-chain constraint.Using
maxStableCoinBalance.balanceinstead of the aggregated sum correctly enforces Pulse's limitation that it cannot combine stables across chains for a single buy.
658-664: LGTM! Clear, actionable warning messages.The error messages are concise and specific:
- The 2 USD minimum is clearly stated
- The gas-fee warning specifies both the native token symbol and the chain name
- The liquidity warning shows the precise maximum amount using the largest single-chain balance
707-707: LGTM! Wallet balance correctly shows aggregated total.Displaying
sumOfStableBalancegives users a complete view of their USDC holdings across all chains.
769-775: LGTM! BuyButton correctly receives all error states.The
notEnoughLiquidityprop now aggregatesminimumStableBalanceandminGasFee, ensuring the button is disabled when any blocking condition is active.
github-actions
bot
temporarily deployed
to
Preview (PRO-3692-Warning-Messages)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
398-401: Use fake timers to avoid the repeated 1.1 s delaysEach warning-message test sleeps for 1.1 s with real timers, adding several seconds to the suite. Switch to
vi.useFakeTimers()/vi.advanceTimersByTime(1100)(orawait waitFor) so the debounce is exercised without slowing the run.Also applies to: 412-414, 439-441, 540-541, 639-640
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pulse/components/Buy/tests/__snapshots__/Buy.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/apps/pulse/components/App/tests/HomeScreen.test.tsx(1 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/hooks/useModularSdk.ts (1)
useModularSdk(18-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (2)
398-400: Replace setTimeout with waitFor for debounced effects.The tests use
setTimeoutwith a hard-coded 1100ms delay to wait for debounced effects. This approach is brittle and can lead to flaky tests on slower CI systems or if the debounce timing changes.Apply this pattern instead:
- // Wait for debounced effect - await new Promise((resolve) => { - setTimeout(resolve, 1100); - }); + // Wait for debounced effect + await waitFor( + () => { + expect(screen.getByText('Min. amount 2 USD')).toBeInTheDocument(); + }, + { timeout: 2000 } + ); - - expect(screen.getByText('Min. amount 2 USD')).toBeInTheDocument();This approach:
- Makes the test more resilient to timing variations
- Fails faster if the expected state never appears
- Decouples the test from implementation timing details
Also applies to: 412-414, 438-441, 539-541, 638-640
464-531: Consider extracting mock data builders.The test suite creates detailed wallet portfolio mock data inline for different scenarios (low stable balance, low gas balance). This leads to significant duplication and makes tests harder to maintain.
Consider extracting helper functions:
const createWalletDataWithBalances = ({ usdcBalance = 10050, ethBalance = 0.5, ethPrice = 3000, }: { usdcBalance?: number; ethBalance?: number; ethPrice?: number; }) => ({ ...mockWalletPortfolioData, result: { ...mockWalletPortfolioData.result, data: { ...mockWalletPortfolioData.result.data, assets: [ // ... construct assets with provided balances ], }, }, });Then in tests:
const lowBalanceWalletData = createWalletDataWithBalances({ usdcBalance: 1, ethBalance: 0.5, });This improves:
- Test readability by focusing on what's different
- Maintainability when the mock data structure changes
- Reusability across test files
Also applies to: 563-630
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Buy/tests/Buy.test.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/hooks/useModularSdk.ts (1)
useModularSdk(18-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (6)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (6)
3-3: LGTM!The addition of
waitForfrom@testing-library/reactand the mock setup forgetDispensableAssetsare well-implemented. The delegation pattern usingmockGetDispensableAssetsprovides good test isolation, and setting a safe default return value prevents undefined errors in tests that don't explicitly mock this function.Also applies to: 45-49, 223-224
134-160: LGTM!The addition of the ETH asset to the mock wallet portfolio data is well-structured and enables testing of gas fee scenarios. Using the zero address (
0x0000...) for the native token is the correct convention.
366-366: LGTM!The updated warning text is more specific and user-friendly, clearly indicating which token is insufficient rather than using a generic "not enough liquidity" message.
646-686: LGTM!The updated module installation test demonstrates good async testing practices:
- Properly mocks dependencies to set up the test scenario
- Explicitly populates
payingTokensby changing the input value- Uses
waitForto wait for the button to reach the expected state before interaction- Verifies both button text and enabled state before triggering the install flow
This approach is more robust and explicit than the previous implementation.
402-402: Verify coupling to UI copy is intentional.The tests assert exact warning message text, which couples them to the specific UI copy. While this ensures users see the correct messages, it means tests will break whenever warning text is updated (even for minor wording improvements).
Consider if this trade-off is acceptable for your team:
- Pro: Tests verify the exact user experience and catch unintended message changes
- Con: Tests require updates for even minor copy improvements
Alternatives if you want to reduce coupling:
- Test for presence of key elements (e.g., warning icon, numeric values) rather than exact text
- Use test IDs for warning message containers
- Extract messages to constants that both tests and components import
Based on typical integration testing practices, asserting exact text for critical user-facing messages is acceptable, but be aware of the maintenance overhead.
Also applies to: 417-418, 444-446, 544-545, 642-642
235-758: Well-structured test suite with comprehensive coverage.The test suite demonstrates good organization and coverage:
- Logical grouping into describe blocks by functionality
- Tests for user interactions, different states, warning scenarios, and edge cases
- Proper setup and teardown with
beforeEach- Good use of modern testing utilities (waitFor, proper mocking patterns)
The new warning message tests align well with the PR objectives and provide good coverage of the updated validation logic.
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests