fixes after QA review on PRO-3679/pulse-preview-screen#400
Conversation
WalkthroughIntroduces stable-currency balance checks and error handling in Buy, updates BuyButton labeling/disabled logic, adds copy-to-clipboard for paying token address, adjusts PreviewBuy header text, refines Sell MAX amount calculation and liquidity handling, extends PayingToken with address, and wires address through intent utils. Tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant B as Buy.tsx
participant PU as Portfolio (assets)
participant I as Intent API
participant BB as BuyButton
U->>B: Enter USD amount / select token
B->>PU: getStableCurrencyBalance()
PU-->>B: Sum(price × balance for STABLE_CURRENCIES)
alt amount > stable balance
B->>B: set insufficientWalletBalance = true
B-->>U: Show "Insufficient wallet balance"
else amount <= stable balance and token selected
B->>I: refreshBuyIntent(debounced amount, token)
I-->>B: intent or notEnoughLiquidity / no routes
alt notEnoughLiquidity or no routes
B-->>U: Show corresponding warning
else intent OK
B-->>U: Show quote details
end
end
B->>BB: Props (states, notEnoughLiquidity || insufficientWalletBalance)
BB-->>U: Button label (spinner | "Buy X SYMBOL for $Y" | "Buy SYMBOL")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: |
b758ec7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84f4a98f.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3679-pulse-preview-scree.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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
66-88: Remove unused “isCopied” state/effect.Copy-to-clipboard for the buy token uses
isBuyTokenAddressCopied.isCopiedis dead code.- const [isCopied, setIsCopied] = useState(false); ... - useEffect(() => { - if (isCopied) { - const timer = setTimeout(() => { - setIsCopied(false); - }, 3000); - return () => clearTimeout(timer); - } - return undefined; - }, [isCopied]);src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
334-347: Test never awaits the click path; also button stays disabled due to empty dispensable assets.
- The inner setTimeout is not awaited; the expect may never run.
- Default mock returns empty arrays, keeping notEnoughLiquidity=true and the button disabled.
Apply this diff to await the debounce and enable the button:
- // Wait for debounced effect and intent response - setTimeout(async () => { - const buyButton = screen.getByRole('button', { name: /Buy/i }); - fireEvent.click(buyButton); - - expect(mockProps.setPreviewBuy).toHaveBeenCalledWith(true); - }, 2000); + // Provide dispensable assets for this test + const intentUtils = await import('../../../utils/intent'); + vi.spyOn(intentUtils, 'getDispensableAssets').mockReturnValue([ + [{ asset: '0x1', chainId: 1, maxValue: '100000000000000000000' }], + [1n], + [mockPayingToken], + ]); + + // Type a valid amount and flush debounce + const input = screen.getByPlaceholderText('0.00'); + fireEvent.change(input, { target: { value: '100.00' } }); + await vi.advanceTimersByTimeAsync(1100); + + const buyButton = await screen.findByRole('button', { name: /Buy/i }); + // Wait until intent resolves and button is enabled + await screen.findByText(/Buy/i); + expect(buyButton).not.toBeDisabled(); + fireEvent.click(buyButton); + expect(mockProps.setPreviewBuy).toHaveBeenCalledWith(true);Also add wait helpers import at top if missing:
import { fireEvent, render, screen } from '@testing-library/react'; +import { waitFor } from '@testing-library/react';
🧹 Nitpick comments (15)
src/apps/pulse/utils/intent.ts (2)
6-7: Return checksummed token addresses in PayingToken for consistency.UI now displays/copies
PayingToken.address. UsegetAddress(...)here so every consumer gets a checksummed address.- address: tokenItem.address, + address: getAddress(tokenItem.address),Also applies to: 67-76
45-66: Guard float math and simplify comparisons.Minor: you can cache
const inputUsd = Number(input) || 0;once and reuse forusdEq > inputUsd, avoiding repeated coercions.src/apps/pulse/components/Sell/Sell.tsx (3)
96-109: Find balance by address+chain across all assets (avoid symbol collisions).Relying on
symbolfirst can fail if multiple assets share a symbol across chains. Search contracts across all assets by(address, chainId)directly.- // Find the asset in the portfolio - const assetData = walletPortfolioData.result.data.assets.find( - (asset) => asset.asset.symbol === token.symbol - ); - if (!assetData) return 0; - - // Find the contract balance for the specific token address and chain - const contractBalance = assetData.contracts_balances.find( - (contract) => - contract.address.toLowerCase() === token.address.toLowerCase() && - contract.chainId === `evm:${token.chainId}` - ); + // Find the contract balance for the specific token address and chain + const contractBalance = walletPortfolioData.result.data.assets + .flatMap((a) => a.contracts_balances) + .find( + (contract) => + contract.address.toLowerCase() === token.address.toLowerCase() && + contract.chainId === `evm:${token.chainId}` + );
330-336: Avoid 10decimals float scaling; floor via string slicing to prevent precision bugs.**
10 ** 18exceeds integer precision;Math.floor(balance * 1e18)can introduce rounding errors. Floor by truncating the fractional part todecimalsdigits as a string.- const decimals = token?.decimals || 18; - const multiplier = 10 ** decimals; - const amount = - Math.floor(tokenBalance * multiplier) / multiplier; - setTokenAmount(amount.toString()); - setParentTokenAmount(amount.toString()); + const decimals = token?.decimals ?? 18; + const [intPart, fracPart = ''] = tokenBalance.toString().split('.'); + const trimmed = decimals > 0 && fracPart ? fracPart.slice(0, decimals) : ''; + const amountStr = trimmed ? `${intPart}.${trimmed}` : intPart; + setTokenAmount(amountStr); + setParentTokenAmount(amountStr);
1-1: Nit: avoid shadowing global parseInt.Importing
parseIntfrom lodash can be confused with the global. Considerimport { parseInt as lodashParseInt }and uselodashParseInt(item).src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
430-446: A11y: make the copy control a button with an aria‑label.Improves keyboard navigation and accessibility without UI changes.
- <CopyToClipboard ...> - <div className="flex items-center ml-1 cursor-pointer"> + <CopyToClipboard ...> + <button + type="button" + aria-label="Copy buy token address" + className="flex items-center ml-1 cursor-pointer bg-transparent" + > {isBuyTokenAddressCopied ? ( <MdCheck className="w-[10px] h-3 text-white" /> ) : ( <img src={CopyIcon} alt="copy-address-icon" className="w-[10px] h-3" /> )} - </div> + </button> </CopyToClipboard>src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (1)
124-130: Works, but prefer role‑based query for the copy button.Querying by
aria-label(after adding it) is more robust thanalton the img.-const copyButtons = screen.getAllByAltText('copy-address-icon'); +const copyButtons = screen.getAllByRole('button', { name: /copy .* address/i });src/apps/pulse/components/Buy/PayingToken.tsx (1)
55-77: Checksum and a11y for the address/copy control.
- Show/copy a checksummed address for consistency.
- Use a button + aria-label for accessibility.
+import { getAddress } from 'viem'; ... + const safeAddress = (() => { + try { + return payingToken.address ? getAddress(payingToken.address) : ''; + } catch { + return payingToken.address || ''; + } + })(); ... - <div className="flex items-center text-[13px] font-normal text-white/50"> - <span> - {payingToken.address - ? `${payingToken.address.slice(0, 6)}...${payingToken.address.slice(-4)}` - : 'Address not available'} - </span> - {payingToken.address && ( + <div className="flex items-center text-[13px] font-normal text-white/50"> + <span> + {safeAddress + ? `${safeAddress.slice(0, 6)}...${safeAddress.slice(-4)}` + : 'Address not available'} + </span> + {safeAddress && ( <CopyToClipboard - text={payingToken.address} + text={safeAddress} onCopy={() => setIsAddressCopied(true)} > - <div className="flex items-center ml-1 cursor-pointer"> + <button + type="button" + aria-label="Copy paying token address" + className="flex items-center ml-1 cursor-pointer bg-transparent" + > {isAddressCopied ? ( <MdCheck className="w-[10px] h-3 text-white" /> ) : ( <img src={CopyIcon} alt="copy-address-icon" className="w-[10px] h-3" /> )} - </div> + </button> </CopyToClipboard> )}src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
268-294: Assertion targets wrong message after first input; verify both banners are cleared.
The first error shown for 999999 is “Insufficient wallet balance”, not “Not enough liquidity”. After changing to 100.00, assert both are absent to avoid false positives.Apply this diff:
- // The liquidity warning should be cleared immediately when user types - expect( - screen.queryByText('Not enough liquidity') - ).not.toBeInTheDocument(); + // Errors should clear immediately when user types + expect(screen.queryByText('Insufficient wallet balance')).not.toBeInTheDocument(); + expect(screen.queryByText('Not enough liquidity')).not.toBeInTheDocument();src/apps/pulse/components/Buy/Buy.tsx (5)
80-113: Optimize and harden stable balance computation.
Current logic re-scans arrays and recomputes on every render; also assumes price/balance are numbers. Memoize and guard undefineds.Apply this diff:
- // Helper function to calculate stable currency balance - const getStableCurrencyBalance = () => { - return ( - 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)) - ) - ) - ) - .reduce((total, 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)) - ) - ); - return ( - total + - stableContracts.reduce( - (sum, contract) => sum + asset.price * contract.balance, - 0 - ) - ); - }, 0) || 0 - ); - }; + // Helper: memoized stable currency USD balance + const stableCurrencyBalance = (() => { + const assets = walletPortfolioData?.result.data.assets ?? []; + if (!assets.length) return 0; + const stableKeys = new Set( + STABLE_CURRENCIES.map( + (s) => `${s.chainId}:${s.address.toLowerCase()}` + ) + ); + let total = 0; + for (const asset of assets) { + const price = Number(asset.price) || 0; + for (const c of asset.contracts_balances ?? []) { + const chainNum = Number(String(c.chainId ?? '').split(':').at(-1)); + const key = `${chainNum}:${String(c.address || '').toLowerCase()}`; + if (stableKeys.has(key)) { + total += price * (Number(c.balance) || 0); + } + } + } + return total; + })();Note: If you prefer, extract to useMemo with assets as a dependency.
198-207: Use memoized stableCurrencyBalance instead of recomputing.
Minor tidy-up and avoids drift in future refactors.Apply this diff:
- const inputAmount = parseFloat(debouncedUsdAmount); - const stableCurrencyBalance = getStableCurrencyBalance(); + const inputAmount = parseFloat(debouncedUsdAmount);And reuse stableCurrencyBalance from above comparison:
- if (inputAmount > stableCurrencyBalance) { + if (inputAmount > stableCurrencyBalance) {(No functional change; remove the local declaration.)
217-220: Early return on notEnoughLiquidity can lock out recovery.
You already reset notEnoughLiquidity in the debounce effect, but guarding here by state may short‑circuit retries if the closure captured true. Prefer relying on current dispensableAssets/permittedChains checks instead.Apply this diff:
- if (notEnoughLiquidity) { - return; - }
271-275: On token change, clear amount and errors to prevent stale state (mirrors Sell behavior).
Matches the team’s pattern from Sell to avoid stale validations when switching tokens.Add a small effect:
useEffect(() => { if (!token) return; setUsdAmount(''); setDebouncedUsdAmount(''); setNoEnoughLiquidity(false); setInsufficientWalletBalance(false); setExpressIntentResponse(null); }, [token]);
449-466: Expose a data-testid for the error banner to stabilize tests.
Text assertions are brittle; provide a stable hook.Apply this diff:
- return ( - <> + return ( + <> <div className="flex items-center justify-center"> <img src={WarningIcon} alt="warning" /> </div> <div + data-testid="pulse-buy-error-banner" style={{ textDecoration: 'underline', color: '#FF366C', fontSize: 12, marginLeft: 5, }} > {message} </div> </> );src/apps/pulse/components/Buy/BuyButton.tsx (1)
121-143: Avoid recomputing isDisabled multiple times.
Hoist to a const to prevent double evaluation and keep styles/label consistent.Apply this diff:
- return ( - <button - className="flex-1 items-center justify-center" - onClick={handleBuySubmit} - disabled={isDisabled()} - style={{ - backgroundColor: isDisabled() ? '#29292F' : '#8A77FF', - color: isDisabled() ? 'grey' : '#FFFFFF', - borderRadius: 8, - }} - type="button" - > - {getButtonText( + const disabled = isDisabled(); + return ( + <button + className="flex-1 items-center justify-center" + onClick={handleBuySubmit} + disabled={disabled} + style={{ + backgroundColor: disabled ? '#29292F' : '#8A77FF', + color: disabled ? 'grey' : '#FFFFFF', + borderRadius: 8, + }} + type="button" + > + {getButtonText( isLoading, isInstalling, isFetching, areModulesInstalled, token, debouncedUsdAmount, - payingTokens.length > 0 ? payingTokens[0] : undefined, - isDisabled() + payingTokens.length > 0 ? payingTokens[0] : undefined, + disabled )} </button> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/apps/pulse/components/App/tests/__snapshots__/AppWrapper.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/App/tests/__snapshots__/HomeScreen.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Buy/tests/__snapshots__/Buy.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Buy/tests/__snapshots__/BuyButton.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Buy/tests/__snapshots__/PreviewBuy.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
src/apps/pulse/components/Buy/Buy.tsx(10 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(2 hunks)src/apps/pulse/components/Buy/PayingToken.tsx(2 hunks)src/apps/pulse/components/Buy/PreviewBuy.tsx(1 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(5 hunks)src/apps/pulse/components/Buy/tests/BuyButton.test.tsx(2 hunks)src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx(4 hunks)src/apps/pulse/components/Sell/Sell.tsx(3 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(1 hunks)src/apps/pulse/types/tokens.ts(1 hunks)src/apps/pulse/utils/intent.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RanaBug
PR: pillarwallet/x#391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
PR: pillarwallet/x#391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Buy/PreviewBuy.tsxsrc/apps/pulse/components/Sell/tests/Sell.test.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/Sell/Sell.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/Buy/Buy.tsx
📚 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/utils/intent.tssrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Buy/BuyButton.tsx
🧬 Code graph analysis (4)
src/apps/pulse/components/Buy/PayingToken.tsx (1)
src/apps/pulse/types/tokens.ts (1)
PayingToken(12-21)
src/apps/pulse/components/Buy/Buy.tsx (1)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)
src/apps/pulse/components/Buy/BuyButton.tsx (2)
src/apps/pulse/types/tokens.ts (2)
SelectedToken(1-10)PayingToken(12-21)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/hooks/useModularSdk.ts (1)
useModularSdk(17-129)
⏰ 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 (16)
src/apps/pulse/components/Sell/Sell.tsx (2)
83-86: Correct: stop fetching when liquidity is insufficient.Short-circuit + explicit reset of local offer prevents stale quotes. Looks good.
277-279: Error message precedence reads well.
relayErrortakes priority, else show liquidity warning. Good UX.src/apps/pulse/components/Sell/tests/Sell.test.tsx (1)
171-172: Test update matches new MAX formatting.Expectation now aligns with precision logic. Good.
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
365-371: Disable Refresh when no dispensable assets.Prevents a no-op call and clarifies UI state.
[suggest_minor_issue]- disabled={ - !buyToken || - !totalPay || - isRefreshingPreview || - isWaitingForSignature - } + disabled={ + !buyToken || + !totalPay || + dispensableAssets.length === 0 || + isRefreshingPreview || + isWaitingForSignature + }src/apps/pulse/components/Buy/tests/BuyButton.test.tsx (1)
69-74: Test refactor to fragment assertions is correct.Matches new button composition.
src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (1)
109-110: Keep copy-icon assertions; count remains two.Still valid with buy header + paying token copy controls.
src/apps/pulse/components/Buy/PayingToken.tsx (1)
20-33: Copy feedback lifecycle is sound.Timeout reset after 3s is appropriate and isolated per item.
src/apps/pulse/components/Buy/tests/Buy.test.tsx (2)
60-61: Mock PayingToken now includes address — good alignment with types.
Matches the PayingToken shape update; no issues.
299-300: Updated zero-balance display to $0.00 — consistent with UI.
Expectation matches new formatting; good.src/apps/pulse/components/Buy/Buy.tsx (5)
71-73: New insufficientWalletBalance state — good separation from liquidity.
Clear distinction between balance vs. routing/liquidity; reads well with the UI messages.
119-121: Reset both error flags on input change — good.
Ensures fresh validation on every keystroke.
496-496: Balance display now reflects stable-currency sum — good UX improvement.
Shows spendable USD rather than total portfolio.
521-522: MAX uses stable-currency balance — correct behavior.
Prevents attempting to spend non-stable assets.
558-559: Disabling Buy on either liquidity or balance errors — good guardrail.
Correctly blocks invalid submissions.src/apps/pulse/components/Buy/BuyButton.tsx (2)
28-35: Spinner on loading/install/fetch — solid UX.
Prevents flicker and shows progress consistently.
41-69: Inline “Buy X SYMBOL for $Y” summary is a nice upgrade.
Formatting via HighDecimalsFormatted and digit limiting reads well.
| address: string; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
PayingToken now requires address — good addition; confirm all producers updated.
This tightens the public contract. Ensure all constructors/mocks across the codebase set address to avoid TS compile breaks at call sites.
Run a quick scan to spot any remaining PayingToken literals missing address:
🏁 Script executed:
#!/bin/bash
rg -n --type=ts -C2 '\bPayingToken\b' | sed -n '1,200p'Length of output: 9168
PayingToken now requires address — update all literals/mocks.
Several test mocks still omit the required address field (this will break TypeScript). Add an address property to all PayingToken literals.
Files to update:
- src/apps/pulse/components/Buy/tests/BuyButton.test.tsx (mockPayingToken)
- src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (mockPayingToken)
- src/apps/pulse/components/Buy/tests/Buy.test.tsx (mockPayingToken)
Also scan the repo for other PayingToken object literals/constructors and add address where missing.
🤖 Prompt for AI Agents
In src/apps/pulse/types/tokens.ts around lines 20 to 21, the PayingToken type
now requires an address: string field; update all PayingToken literals/mocks to
include a valid address string. Specifically, add an address property to
mockPayingToken in src/apps/pulse/components/Buy/tests/BuyButton.test.tsx,
PreviewBuy.test.tsx, and Buy.test.tsx, and scan the repo for any other
PayingToken object literals or constructors to add a suitable address value (can
be a test ENS or hex like "0x000...") so TypeScript no longer errors on missing
address.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests