Conversation
…UI improvements on search modal
…I, percentage buttons etc.
WalkthroughIntegrates Relay sell flows and wallet portfolio into Pulse: adds Relay SDK and hooks, portfolio-driven token selection and UI (PortfolioTokenList, Sell, PreviewSell, SellButton), Loading/Refresh contexts, App/HomeScreen plumbing, UI/styling tweaks, and dependency updates (viem pinning, relay-sdk, styled-components resolution). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant AW as AppWrapper
participant TK as useTransactionKit
participant P as Wallet Portfolio API
participant HS as HomeScreen
participant S as Search
participant PTL as PortfolioTokenList
U->>AW: Open Pulse
AW->>TK: request accountAddress
TK-->>AW: accountAddress
AW->>P: fetch wallet portfolio (accountAddress)
P-->>AW: walletPortfolioData
AW->>HS: render HomeScreen(with portfolio & sell state)
HS->>S: render Search(with portfolio props)
U->>S: choose "My Holdings"
S->>PTL: render holdings list
U->>PTL: select token
PTL-->>S: handleTokenSelect(token)
S-->>HS: setSellToken / setBuyToken
sequenceDiagram
autonumber
actor U as User
participant HS as HomeScreen
participant SL as Sell
participant R as useRelaySell
participant RS as Relay API
participant PB as PreviewSell
participant TX as Relay execution
U->>HS: switch to Sell
HS->>SL: render Sell(token)
U->>SL: enter amount
SL->>R: getBestSellOffer(params) (debounced)
R->>RS: request quote/route
RS-->>R: offer
R-->>SL: SellOffer
U->>SL: click Sell
SL-->>HS: setSellOffer + open preview
HS->>PB: render PreviewSell
U->>PB: confirm
PB->>R: executeSell(token, amount)
R->>TX: build & submit batch
TX-->>R: txHash / result
R-->>PB: success or error
PB-->>U: show tx result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 warnings)❌ Failed checks (3 warnings)
Poem
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.
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. ✨ Finishing Touches
🧪 Generate unit tests
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: |
e0eb063
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://522bbc56.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3638-sell-feature-p.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: 15
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/Search/Search.tsx (2)
124-140: Abort in-flight fetches to avoid race conditions and setState after unmount.Rapid tab/chain changes can race responses; add AbortController and cleanup.
- useEffect(() => { + useEffect(() => { if (searchType) { - setIsLoading(true); - setParsedAssets(undefined); - fetch(`${getUrl(searchType)}?chainIds=${getChainId(chains)}`) + const controller = new AbortController(); + setIsLoading(true); + setParsedAssets(undefined); + fetch(`${getUrl(searchType)}?chainIds=${getChainId(chains)}`, { + signal: controller.signal, + }) .then((response) => response.json()) .then((result) => { const assets = parseFreshAndTrendingTokens(result.projection); setParsedAssets(assets); setIsLoading(false); }) .catch((err) => { - setIsLoading(false); - console.error(err); + if (err?.name !== 'AbortError') { + setIsLoading(false); + console.error(err); + } }); } - }, [searchType, chains]); + return () => controller.abort(); + }, [searchType, chains]);
100-108: Implement TopGainers endpoint in getUrl
In src/apps/pulse/components/Search/Search.tsx (lines 100–108), add aSearchType.TopGainersbranch with its proper testnet/production URL—currently it falls back to Trending.
🧹 Nitpick comments (28)
package.json (2)
162-166: Remove stale react-scripts override; unify on one package-manager mechanism.There’s no react-scripts in deps; keeping its override is noise. Also, having both npm “overrides” and Yarn “resolutions” complicates resolution.
Apply:
"overrides": { - "react-scripts": { - "typescript": "^5" - }, "viem": "^2.37.1" },And prefer a single strategy (overrides for npm/pnpm or resolutions for Yarn) across the repo.
34-35: Bundle impact of @relayprotocol/relay-sdk.If the SDK pulls Node polyfills, ensure Vite 7 has needed shims or that the SDK is ESM/browser-ready. Consider lazy importing Sell flow to keep initial bundle small.
src/apps/faq-app/index.tsx (1)
3-3: Mobile viewport nit: prefer 100dvh to avoid browser-UI jumps.- <div style={{ width: '100%', height: '100vh' }}> + <div style={{ width: '100%', height: '100dvh' }}>src/apps/pulse/components/Misc/Refresh.tsx (1)
5-11: Icon-only button a11y: hide decorative img and add focus ring.Screen readers will already announce aria-label; hide the img and add a visible focus style.
- <button - className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px]" + <button + className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px] focus:outline-none focus-visible:ring-2 focus-visible:ring-white/40" type="button" aria-label="Refresh" > - <img src={RefreshIcon} alt="refresh-icon" /> + <img src={RefreshIcon} alt="" aria-hidden="true" /> </button>src/apps/pulse/components/Misc/Esc.tsx (1)
9-16: Add keyboard Escape handling and focus style.Improve accessibility so keyboard users can dismiss with Esc and see focus.
- <button - className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px]" - onClick={closePreview} + <button + className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px] focus:outline-none focus-visible:ring-2 focus-visible:ring-white/40" + onClick={closePreview} + onKeyDown={(e) => { + if (e.key === 'Escape') closePreview(); + }} type="button" - aria-label="Close" + aria-label="Close" > <p className="text-white/50 font-medium text-[13px] leading-[100%] tracking-[-0.02em] text-center"> ESC </p>src/apps/pulse/hooks/useRelaySdk.ts (2)
31-43: Minor: hoist chains to a constant to avoid re-allocations.+const RELAY_CHAINS = [ + convertViemChainToRelayChain(mainnet), + convertViemChainToRelayChain(polygon), + convertViemChainToRelayChain(base), + convertViemChainToRelayChain(arbitrum), + convertViemChainToRelayChain(optimism), + convertViemChainToRelayChain(bsc), + convertViemChainToRelayChain(gnosis), +]; ... - chains: [ - convertViemChainToRelayChain(mainnet), - convertViemChainToRelayChain(polygon), - convertViemChainToRelayChain(base), - convertViemChainToRelayChain(arbitrum), - convertViemChainToRelayChain(optimism), - convertViemChainToRelayChain(bsc), - convertViemChainToRelayChain(gnosis), - ], + chains: RELAY_CHAINS,
32-33: Make base API configurable per environment.Hard-coding MAINNET_RELAY_API limits staging/dev. Consider env-driven selection.
Example:
const baseApiUrl = import.meta.env.VITE_RELAY_API_URL ?? MAINNET_RELAY_API;src/apps/pulse/hooks/useRelaySell.ts (4)
137-138: Clamp slippage to a sane range and compute BPS safely.Guard against negative or >100% slippage inputs to avoid invalid requests.
- slippage = 0.03, + slippage = 0.03, @@ - slippageTolerance: Math.floor(slippage * 10000), // Convert to basis points + slippageTolerance: Math.min( + 10_000, + Math.max(0, Math.floor(slippage * 10_000)) + ),Also applies to: 185-187
442-461: Approval spender derivation is brittle; prefer explicit data from the quote.Heuristics that pick the first
item.data.tonot equal to the token can select the wrong spender. If Relay exposes an approval step or an approval target in the quote details, use that; otherwise, fall back with stronger checks.I recommend verifying Relay’s approval metadata (step id names and any
approval/spenderfields) and switching to that source. If needed, I can adapt the code once you confirm the exact field.Also applies to: 528-546
278-305: Comment says “actual USDC received”, but the fee is based on the quoted amount.This may be acceptable, but the wording is misleading and can overcharge slightly on high slippage routes. Either (a) update the comment to reflect quote-based calculation or (b) add a post-swap
balanceOfread and compute 1% on the realized amount.
813-865: Transaction metadata: include chain name/token symbol in batch name for clarity.Minor UX improvement for Wallet activity and debugging.
Example:
- batchName:
pulse-sell-${token.symbol}-${token.chainId}- title: include chain short name (e.g., “on Base”)
src/apps/pulse/components/Buy/Buy.tsx (1)
311-327:no-spinnerhas no effect ontype="text".That class typically targets number inputs. Either switch to
type="number"with proper validation or remove the class.- <input - className="no-spinner" - placeholder={inputPlaceholder} + <input + className="" + placeholder={inputPlaceholder}src/apps/pulse/components/App/AppWrapper.tsx (1)
45-56: Avoid duplicate portfolio queries across AppWrapper and HomeScreen.HomeScreen also calls
useGetWalletPortfolioQuery, causing two identical requests. Consider lifting the data to AppWrapper and passing it down to HomeScreen to reuse.Example (AppWrapper):
- <HomeScreen + <HomeScreen setSearching={setSearching} buyToken={buyToken} sellToken={sellToken} isBuy={isBuy} setIsBuy={setIsBuy} + walletPortfolioData={walletPortfolioData?.result?.data} />And in HomeScreen, accept
walletPortfolioDataprop and remove its internal query.Also applies to: 58-64
src/apps/pulse/components/App/HomeScreen.tsx (2)
52-55: Duplicate portfolio fetch.As noted in AppWrapper, consider accepting
walletPortfolioDataas a prop to avoid a second query here.
81-103: Rendering helper is clean; minor readability nit.
renderPreviewclosure is fine; alternatively, inline conditional blocks can simplify without changing behavior. Optional.Also applies to: 199-218
src/apps/pulse/components/Search/Search.tsx (1)
167-175: Avoid hardcoding dailyPriceChange = -0.02.Populate from API when available or default to 0 to prevent misleading UI.
- dailyPriceChange: -0.02, + dailyPriceChange: item.dailyPriceChange ?? 0,(Apply similarly in the sell path.)
Also applies to: 184-191, 195-204
src/apps/pulse/components/Sell/SellButton.tsx (3)
1-1: Type getButtonText to return ReactNode.Explicit return type avoids mixed string/JSX inference issues.
-import { Dispatch, SetStateAction } from 'react'; +import { Dispatch, SetStateAction, ReactNode } from 'react'; @@ -function getButtonText( +function getButtonText( isLoading: boolean, isInitialized: boolean, selectedToken: SelectedToken | null, tokenAmount: string, sellOffer?: SellOffer | null -) { +): ReactNode {Also applies to: 10-16
67-77: Compute disabled state once to avoid redundant calls.Small readability/perf win.
- const isDisabled = () => { - return ( + const disabled = + ( isLoadingOffer || !token || !(parseFloat(tokenAmount) > 0) || !sellOffer || sellOffer.tokenAmountToReceive === 0 || notEnoughLiquidity || !isInitialized - ); - }; + ); @@ - disabled={isDisabled()} + disabled={disabled} @@ - backgroundColor: isDisabled() ? '#29292F' : '#8A77FF', - color: isDisabled() ? 'grey' : '#FFFFFF', + backgroundColor: disabled ? '#29292F' : '#8A77FF', + color: disabled ? 'grey' : '#FFFFFF',Also applies to: 81-88, 83-83
60-65: Redundant setSellOffer call on click.sellOffer is already in props; unless parent requires a re-set, this is unnecessary.
- const handleSellClick = () => { - if (sellOffer) { - setSellOffer(sellOffer); - setPreviewSell(true); - } - }; + const handleSellClick = () => { + if (sellOffer) { + setPreviewSell(true); + } + };src/apps/pulse/components/Sell/PreviewSell.tsx (2)
305-311: Show a precise rate derived from the current quote.Use sellOffer/tokenAmount to compute 1 token ≈ X USDC; fallback to usdValue only if needed.
- {detailsEntry( - 'Rate', - `1 ${sellToken?.symbol} ≈ ${sellToken?.usdValue ? Number(sellToken.usdValue).toFixed(3) : 1.0}`, - false, - 'USDC' - )} + {detailsEntry( + 'Rate', + (() => { + const amt = parseFloat(tokenAmount); + const rate = + !Number.isNaN(amt) && amt > 0 + ? sellOffer.tokenAmountToReceive / amt + : Number(sellToken?.usdValue ?? 1); + return `1 ${sellToken?.symbol} ≈ ${rate.toFixed(3)}`; + })(), + false, + 'USDC' + )}
201-219: Minor accessibility: improve alt text for token logo.Use a descriptive alt like
${sellToken?.symbol} logo.- alt="Main" + alt={`${sellToken?.symbol ?? 'token'} logo`}src/apps/pulse/components/Sell/Sell.tsx (4)
236-244: Render a readable Relay error message.
relayErrormay be an Error object; rendering it directly prints “[object Object]”.Apply this diff:
- <div className="underline text-[#FF366C] text-xs ml-1.5"> - {relayError || 'Not enough liquidity'} - </div> + <div className="underline text-[#FF366C] text-xs ml-1.5"> + {typeof relayError === 'string' + ? relayError + : relayError?.message || 'Not enough liquidity'} + </div>
211-223: Improve numeric input UX and validation.Guide mobile keyboards and restrict characters to decimals. Keeps current parsing intact.
Apply this diff:
<input className="w-full no-spinner text-right bg-transparent outline-none pr-0" style={{ fontSize: '36px', lineHeight: '1.2', fontWeight: '500', }} placeholder={inputPlaceholder} onChange={handleTokenAmountChange} value={tokenAmount} - type="text" + type="text" + inputMode="decimal" + pattern="^[0-9]*[.,]?[0-9]*$" + autoComplete="off" onFocus={() => setInputPlaceholder('')} />
54-77: Guard against out-of-order offer responses.Rapid edits can resolve promises out of order and set stale offers. Track a request id and only apply the latest response.
Example patch (conceptual):
+ const reqRef = useRef(0); const fetchSellOffer = useCallback(async () => { if ( debouncedTokenAmount && token && isInitialized && parseFloat(debouncedTokenAmount) > 0 && !notEnoughLiquidity ) { setIsLoadingOffer(true); try { + const myReq = ++reqRef.current; const offer = await getBestSellOffer({ fromAmount: debouncedTokenAmount, fromTokenAddress: token.address, fromChainId: token.chainId, fromTokenDecimals: token.decimals, }); - setLocalSellOffer(offer); + if (reqRef.current === myReq) setLocalSellOffer(offer); } catch (error) { console.error('Failed to fetch sell offer:', error); setLocalSellOffer(null); } finally { setIsLoadingOffer(false); }
159-163: Nit: improve image alt text for accessibility.Use a descriptive alt using symbol or name.
Apply this diff:
- <img - src={token.logo} - alt="Main" + <img + src={token.logo} + alt={`${token.symbol} logo`}src/apps/pulse/components/Search/PortfolioTokenList.tsx (3)
47-71: Memoize filtered/sorted tokens to avoid recomputation on every render.
convertPortfolioAPIResponseToToken+ filter + sort can be heavy for large portfolios; memoize by inputs.Apply this diff:
+import { useMemo } from 'react'; @@ - const portfolioTokens = getFilteredPortfolioTokens(); + const portfolioTokens = useMemo(getFilteredPortfolioTokens, [ + walletPortfolioData, + searchText, + ]);Also applies to: 73-74
38-45: Nit: guard against unexpected casing and missing fields in stability check.Defensive coding avoids crashes if upstream fields vary.
Apply this diff:
- const isStableCurrency = (token: Token) => { + const isStableCurrency = (token: Token) => { const chainId = chainNameToChainIdTokensData(token.blockchain); return STABLE_CURRENCIES.some( (stable) => - stable.chainId === chainId && - stable.address.toLowerCase() === token.contract.toLowerCase() + stable.chainId === chainId && + stable.address.toLowerCase() === token.contract?.toLowerCase() ); };
176-184: A11y nit: clarify alt text for chain logo.Include chain name in alt for screen readers.
Apply this diff:
- <img + <img src={getLogoForChainId( chainNameToChainIdTokensData(token.blockchain) )} className="absolute -bottom-px -right-px w-3.5 h-3.5 rounded-full border-[0.88px] border-[#25232D]" - alt="chain logo" + alt={`${token.blockchain || 'EVM'} chain logo`} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.json(3 hunks)src/apps/faq-app/index.tsx(1 hunks)src/apps/pulse/components/App/AppWrapper.tsx(3 hunks)src/apps/pulse/components/App/HomeScreen.tsx(1 hunks)src/apps/pulse/components/Buy/Buy.tsx(2 hunks)src/apps/pulse/components/Buy/PreviewBuy.tsx(3 hunks)src/apps/pulse/components/Misc/Esc.tsx(1 hunks)src/apps/pulse/components/Misc/Refresh.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(1 hunks)src/apps/pulse/components/Search/Search.tsx(9 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(1 hunks)src/apps/pulse/components/Sell/Sell.tsx(2 hunks)src/apps/pulse/components/Sell/SellButton.tsx(1 hunks)src/apps/pulse/hooks/useRelaySdk.ts(1 hunks)src/apps/pulse/hooks/useRelaySell.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/App/AppWrapper.tsxsrc/apps/pulse/components/Search/Search.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/App/AppWrapper.tsx
🧬 Code graph analysis (8)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (5)
src/types/api.ts (1)
PortfolioData(739-748)src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(1-9)src/utils/blockchain.ts (1)
getLogoForChainId(158-192)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/components/Sell/PreviewSell.tsx (8)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (2)
SellOffer(39-42)useRelaySell(52-913)src/types/api.ts (1)
WalletPortfolioMobulaResponse(750-752)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(3-13)src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(5-19)src/utils/blockchain.ts (1)
getLogoForChainId(158-192)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/components/App/AppWrapper.tsx (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
HomeScreen(31-232)
src/apps/pulse/hooks/useRelaySell.ts (8)
src/apps/pulse/hooks/useRelaySdk.ts (1)
useRelaySdk(21-59)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(60-79)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(1-9)src/apps/the-exchange/utils/wrappedTokens.ts (3)
getWrappedTokenAddressIfNative(33-44)isWrappedToken(23-28)isNativeToken(30-31)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
Token(19-29)src/apps/the-exchange/utils/blockchain.ts (2)
getNativeBalanceFromPortfolio(129-141)toWei(124-126)
src/apps/pulse/components/Sell/SellButton.tsx (2)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(39-42)
src/apps/pulse/components/Sell/Sell.tsx (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/types/api.ts (1)
WalletPortfolioMobulaResponse(750-752)src/apps/pulse/hooks/useRelaySell.ts (2)
SellOffer(39-42)useRelaySell(52-913)src/utils/blockchain.ts (1)
getLogoForChainId(158-192)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/components/Search/Search.tsx (3)
src/types/api.ts (1)
PortfolioData(739-748)src/apps/pulse/hooks/useTokenSearch.ts (1)
useTokenSearch(5-42)src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)
src/apps/pulse/components/App/HomeScreen.tsx (2)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(39-42)
🪛 Biome (2.1.2)
src/apps/pulse/components/Sell/Sell.tsx
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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 (8)
package.json (1)
96-96: Viem bump OK ^2.37.1 satisfies wagmi@2.14.16’s viem 2.x peerDependency; no other transitive packages require viem.src/apps/pulse/components/Buy/Buy.tsx (1)
193-199: Padding change looks good.The denser container (
p-3) improves alignment with the updated header chrome.src/apps/pulse/components/Buy/PreviewBuy.tsx (2)
100-110: UI polish LGTM.Consistent tile styling for Refresh/Esc and added icons improve clarity without altering logic.
218-231: Nice touch adding an accessible copy button.
aria-label="Copy address"and visual affordance are correctly applied.src/apps/pulse/components/App/AppWrapper.tsx (1)
19-28: Good: portfolio query gated by account address.Skip logic avoids unnecessary requests when disconnected.
src/apps/pulse/components/App/HomeScreen.tsx (1)
31-41: Sell preview state wiring looks correct.Clear reset of
sellOfferandtokenAmounton close prevents stale data.src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
147-159: LGTM: row keying and click handling are correct.
7-13: No mismatched blockchain names; mapping coverage confirmed. The converter uses chainIdToChainNameTokensData to emit chain names (e.g. “Optimistic”) that exactly match the keys in chainNameToChainIdTokensData, so there’s no “Optimism” vs “Optimistic” mismatch.
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3638/sell-feature-pulse)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
src/apps/pulse/components/Sell/Sell.tsx (5)
131-140: Prevent stale offers when input is cleared (debounce should also emit '').useEffect(() => { const timer = setTimeout(() => { - if (tokenAmount && !Number.isNaN(parseFloat(tokenAmount))) { - setDebouncedTokenAmount(tokenAmount); - } + const safe = + tokenAmount && !Number.isNaN(parseFloat(tokenAmount)) + ? tokenAmount + : ''; + setDebouncedTokenAmount(safe); }, 1000); return () => clearTimeout(timer); }, [tokenAmount]);
113-130: Recalculate liquidity on token or balance changes, not only on input change.const tokenBalance = getTokenBalance(); + useEffect(() => { + if (token && tokenAmount) { + const inputAmount = parseFloat(tokenAmount); + setNotEnoughLiquidity( + !Number.isNaN(inputAmount) && inputAmount > tokenBalance + ); + } else { + setNotEnoughLiquidity(false); + } + }, [token, tokenBalance, tokenAmount]); + const handleTokenAmountChange = (e: React.ChangeEvent<HTMLInputElement>) => {
1-1: Fix Biome lint: stop shadowing global parseInt.Remove the lodash import and use Number.parseInt where needed.
-import { parseInt } from 'lodash'; +// removed: avoid shadowing global parseInt; use Number.parseInt(item, 10)
291-291: Use Number.parseInt with radix (and remove lodash import).- const percentage = isMax ? 100 : parseInt(item); + const percentage = isMax ? 100 : Number.parseInt(item, 10);
93-106: Balance lookup can mis-match on duplicate symbols—match by address+chain across all assets.- // 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}` - ); - return contractBalance?.balance || 0; + // Find the contract balance by exact address+chain across all assets + const contractBalance = walletPortfolioData.result.data.assets + .flatMap((asset) => asset.contracts_balances || []) + .find( + (contract) => + contract?.address?.toLowerCase() === token.address.toLowerCase() && + contract?.chainId === `evm:${token.chainId}` + ); + return contractBalance?.balance ?? 0;src/apps/pulse/hooks/useRelaySell.ts (4)
276-281: Avoid duplicate wrapping when the route already includes a wrap.- const isWrapRequired = + let isWrapRequired = isWrappedToken( sellOffer.offer.details?.currencyIn?.currency?.address || '', token.chainId ) && !isWrappedToken(token.address, token.chainId); + + // Detect wrap already present in route + const wrappedTokenAddressForDetect = getWrappedTokenAddressIfNative( + token.address, + token.chainId + ); + const hasWrapInRoute = + steps.some((s) => s.id === 'wrap') || + steps.some((s) => + s.items?.some( + (it) => + it.data?.to?.toLowerCase() === + wrappedTokenAddressForDetect?.toLowerCase() && + (it.data?.value ?? '0') !== '0' + ) + ); + isWrapRequired = isWrapRequired && !hasWrapInRoute;
2-2: Use the initialized Relay client instance; avoid getClient singleton.Prevents races with SDK initialization and aligns with useRelaySdk.
-import { Execute, getClient } from '@relayprotocol/relay-sdk'; +import { Execute } from '@relayprotocol/relay-sdk'; @@ -export default function useRelaySell() { - const { isInitialized, accountAddress } = useRelaySdk(); +export default function useRelaySell() { + const { isInitialized, accountAddress, relayClient } = useRelaySdk(); @@ - // Get quote from Relay SDK - const client = getClient(); - const quote = await client.actions.getQuote(quoteRequest); + // Get quote from Relay SDK + if (!relayClient) { + setError('Relay SDK is not initialized. Please try again.'); + return null; + } + const quote = await relayClient.actions.getQuote(quoteRequest);Also applies to: 55-55, 195-197
5-11: Validate fee receiver address (format and non-zero).Fail fast on invalid config to avoid executing a bad transfer.
import { createPublicClient, encodeFunctionData, erc20Abi, formatUnits, http, parseUnits, + isAddress, } from 'viem'; @@ const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; // Validate fee receiver address if (!feeReceiver) { throw new Error('Fee receiver address is not configured'); } + if (!isAddress(feeReceiver) || isZeroAddress(feeReceiver)) { + throw new Error('Invalid fee receiver address configured'); + }Also applies to: 289-295
478-482: Fix allowance comparison: compare BigInt amounts, not formatted strings.String comparisons can be wrong; use raw BigInt.
- const isEnoughAllowance = isAllowance - ? formatUnits(isAllowance, token.decimals) >= - formatUnits(fromAmountBigInt, token.decicals) - : undefined; + const isEnoughAllowance = + isAllowance !== undefined + ? isAllowance >= fromAmountBigInt + : undefined;(Apply in both allowance checks.)
Also applies to: 563-567
🧹 Nitpick comments (10)
src/apps/pulse/contexts/RefreshContext.tsx (1)
2-8: Remove lint-disable by memoizing the context valueMemoize the provider value to avoid re-renders and drop the eslint suppression.
-import { - ReactNode, - createContext, - useCallback, - useContext, - useState, -} from 'react'; +import { ReactNode, createContext, useCallback, useContext, useMemo, useState } from 'react'; - return ( - <RefreshContext.Provider - value={{ - isRefreshing, - refreshSell, - refreshPreviewSell, - setRefreshSellCallback, - setRefreshPreviewSellCallback, - }} - > + const value = useMemo( + () => ({ + isRefreshing, + refreshSell, + refreshPreviewSell, + setRefreshSellCallback, + setRefreshPreviewSellCallback, + }), + [isRefreshing, refreshSell, refreshPreviewSell] + ); + + return ( + <RefreshContext.Provider value={value}> {children} </RefreshContext.Provider> );Also applies to: 55-63
src/apps/pulse/components/Misc/Refresh.tsx (1)
23-31: A11y: mark decorative icon hidden and expose busy stateThe button already has
aria-label. Hide the decorative image and reflect loading state.- aria-label="Refresh" + aria-label="Refresh" + aria-busy={isLoading} onClick={onClick} disabled={disabled || isLoading} > {isLoading ? ( - <TailSpin color="#FFFFFF" height={20} width={20} /> + <TailSpin color="#FFFFFF" height={20} width={20} /> ) : ( - <img src={RefreshIcon} alt="refresh-icon" /> + <img src={RefreshIcon} alt="" aria-hidden="true" /> )}src/apps/pulse/contexts/LoadingContext.tsx (1)
1-3: Stabilize provider value; avoid wrapper; drop lint-disableUse
useMemoand pass React’s stable state setter directly.-/* eslint-disable react/jsx-no-constructed-context-values */ -import { ReactNode, createContext, useContext, useState } from 'react'; +import { ReactNode, createContext, useContext, useMemo, useState } from 'react'; @@ - const setQuoteLoading = (loading: boolean) => { - setIsQuoteLoading(loading); - }; - - return ( - <LoadingContext.Provider - value={{ - isQuoteLoading, - setQuoteLoading, - }} - > + const value = useMemo( + () => ({ + isQuoteLoading, + setQuoteLoading: setIsQuoteLoading, + }), + [isQuoteLoading] + ); + + return ( + <LoadingContext.Provider value={value}> {children} </LoadingContext.Provider> );Also applies to: 18-31
src/apps/pulse/components/App/AppWrapper.tsx (3)
19-19: Remove unnecessary eslint-disable
sellTokenis used later; the suppression can be dropped.- // eslint-disable-next-line @typescript-eslint/no-unused-vars
47-73: Avoid double-fetching portfolio data; pass it to HomeScreen
HomeScreenfetches portfolio again. Prefer lifting the query here and passing data/loading/error toHomeScreento reduce network calls and keep a single source of truth.Illustrative changes:
- <HomeScreen + <HomeScreen setSearching={setSearching} buyToken={buyToken} sellToken={sellToken} isBuy={isBuy} setIsBuy={setIsBuy} + walletPortfolioData={walletPortfolioData?.result?.data} + walletPortfolioLoading={walletPortfolioLoading} + walletPortfolioError={!!walletPortfolioError} />Then, remove the duplicate query in
HomeScreenand consume these props. I can provide a follow-up patch forHomeScreenPropsif you’d like.
39-45: Effect dependency uses unstable object; depend on the search string instead
queryis a new object each render, which can retrigger the effect unnecessarily. DeriveassetfromuseLocation().searchand depend on the string.- const useQuery = () => { - const { search } = useLocation(); - return new URLSearchParams(search); - }; - const query = useQuery(); + const { search } = useLocation(); @@ - }, [setSearching, query]); + }, [setSearching, search]);src/apps/pulse/components/App/HomeScreen.tsx (3)
58-61: Remove duplicate portfolio query; consume data from AppWrapper
useGetWalletPortfolioQueryis already run inAppWrapper. Fetching again here adds latency and potential inconsistency. AcceptwalletPortfolioData(and flags) via props instead and delete this query block. I can provide a concrete patch updatingHomeScreenPropsand usages on request.
88-110: A11y: decorative search icon should be hiddenThe button has text. Hide the icon from assistive tech.
- <span style={{ marginLeft: 10 }}> - <img src={SearchIcon} alt="search-icon" /> - </span> + <span style={{ marginLeft: 10 }}> + <img src={SearchIcon} alt="" aria-hidden="true" /> + </span>
219-234: Keep portfolio data shape consistent across componentsHere
walletPortfolioDatais passed raw, whileAppWrapperpasseswalletPortfolioData?.result?.datatoSearch. Standardize the shape (either unwrap in the provider or at call sites) to avoid mismatches and defensive?.chains everywhere.src/apps/pulse/components/Sell/Sell.tsx (1)
273-281: Guard against NaN when showing USD estimate.Consider coercing usdValue safely to number to avoid NaN rendering if backend changes format:
- {limitDigitsNumber(tokenBalance * parseFloat(token.usdValue))} + {limitDigitsNumber(tokenBalance * Number(token.usdValue || 0))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/apps/pulse/components/App/AppWrapper.tsx(3 hunks)src/apps/pulse/components/App/HomeScreen.tsx(1 hunks)src/apps/pulse/components/Misc/Refresh.tsx(1 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(1 hunks)src/apps/pulse/components/Sell/Sell.tsx(2 hunks)src/apps/pulse/contexts/LoadingContext.tsx(1 hunks)src/apps/pulse/contexts/RefreshContext.tsx(1 hunks)src/apps/pulse/hooks/useRelaySell.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Sell/PreviewSell.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/App/AppWrapper.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/App/AppWrapper.tsxsrc/apps/pulse/components/Sell/Sell.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/hooks/useRelaySell.ts
🧬 Code graph analysis (4)
src/apps/pulse/components/App/AppWrapper.tsx (3)
src/apps/pulse/contexts/LoadingContext.tsx (1)
LoadingProvider(15-32)src/apps/pulse/contexts/RefreshContext.tsx (1)
RefreshProvider(24-67)src/apps/pulse/components/Search/Search.tsx (1)
Search(57-381)
src/apps/pulse/components/App/HomeScreen.tsx (6)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/contexts/RefreshContext.tsx (1)
useRefresh(69-75)src/apps/pulse/contexts/LoadingContext.tsx (1)
useLoading(34-40)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(42-45)src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
PreviewBuy(25-320)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-34)
src/apps/pulse/components/Sell/Sell.tsx (6)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/types/api.ts (1)
WalletPortfolioMobulaResponse(750-752)src/apps/pulse/hooks/useRelaySell.ts (2)
SellOffer(42-45)useRelaySell(55-920)src/apps/pulse/contexts/RefreshContext.tsx (1)
useRefresh(69-75)src/utils/blockchain.ts (1)
getLogoForChainId(158-192)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/hooks/useRelaySell.ts (9)
src/apps/pulse/hooks/useRelaySdk.ts (1)
useRelaySdk(21-59)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/pulse/contexts/LoadingContext.tsx (1)
useLoading(34-40)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(60-79)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(1-9)src/apps/the-exchange/utils/wrappedTokens.ts (3)
getWrappedTokenAddressIfNative(33-44)isWrappedToken(23-28)isNativeToken(30-31)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
Token(19-29)src/apps/the-exchange/utils/blockchain.ts (2)
getNativeBalanceFromPortfolio(129-141)toWei(124-126)
🪛 Biome (2.1.2)
src/apps/pulse/components/Sell/Sell.tsx
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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 (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
75-86: Callbacks registered on mount in Sell and PreviewSellConfirmed that
setRefreshSellCallbackis invoked in Sell.tsx (lines 156–158) andsetRefreshPreviewSellCallbackin PreviewSell.tsx (lines 107–110), so bothrefreshSellandrefreshPreviewSellcallbacks are registered on component mount—before any Refresh button becomes enabled.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Great work! Just a few notes and minor changes
| const { | ||
| data: walletPortfolioData, | ||
| isLoading: walletPortfolioLoading, | ||
| error: walletPortfolioError, | ||
| } = useGetWalletPortfolioQuery( | ||
| { wallet: accountAddress || '', isPnl: false }, | ||
| { skip: !accountAddress } | ||
| ); |
There was a problem hiding this comment.
Can i just check that this doesn't refresh on focus?
There was a problem hiding this comment.
No there is no options refetchOnFocus so it should not
There was a problem hiding this comment.
Ah wait we have setupListeners(store.dispatch) for PillarX which apparently will automatically refetchOnFocus for all RTK queries. Changing this for Pulse only for now.
| if (!sellToken || !sellOffer) { | ||
| return ( | ||
| <div className="flex flex-col items-center justify-center min-h-screen bg-black"> | ||
| <div className="text-white text-lg">Invalid data</div> |
There was a problem hiding this comment.
Can we use a better message here please
| <div className="flex items-center"> | ||
| <div className="relative inline-block mr-2"> | ||
| <div className="w-8 h-8 overflow-hidden rounded-full flex items-center justify-center bg-[#2775CA]"> | ||
| <span className="text-white font-bold text-sm">US</span> |
There was a problem hiding this comment.
Is this supposed to be USD?
| <div className="flex items-center text-xs font-normal text-white/50"> | ||
| <span> | ||
| {usdcAddress | ||
| ? `${usdcAddress.slice(0, 6)}...${usdcAddress.slice(-4)}` |
There was a problem hiding this comment.
Please check how this appears on mobile / small screens - might be better to use ellipsis overflow
There was a problem hiding this comment.
The problem is, at the moment the Pulse app is not really mobile friendly. But this component looks good for mobile, the address here is no issue
| try { | ||
| if (!token || !walletPortfolioData?.result?.data?.assets) return 0; | ||
|
|
||
| // 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}` | ||
| ); | ||
| return contractBalance?.balance || 0; | ||
| } catch (error) { | ||
| console.error('Error getting token balance:', error); | ||
| return 0; | ||
| } | ||
| }; |
There was a problem hiding this comment.
@RanaBug same question as above as this may not be relevant in our case
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
package.json (1)
93-94: Align styled-components to v6 and remove leftover v5 DT types.You switched to styled-components v6 and resolution ^6, but still ship @types/styled-components v5, which can cause TS conflicts. Remove the v5 DT package.
"devDependencies": { @@ - "@types/styled-components": "^5.1.34",Also applies to: 167-169, 119-119
src/apps/pulse/components/App/HomeScreen.tsx (1)
204-213: Disable Refresh correctly in Sell mode when no sell token is selected.Current check enables Refresh if buyToken exists but sellToken is missing. Gate by active mode.
- <Refresh - onClick={isBuy ? undefined : handleRefresh} - isLoading={isBuy ? false : isQuoteLoading || isRefreshing} - disabled={ - isBuy || - isQuoteLoading || - isRefreshing || - (!buyToken && !sellToken) - } - /> + <Refresh + onClick={isBuy ? undefined : handleRefresh} + isLoading={isBuy ? false : isQuoteLoading || isRefreshing} + disabled={ + isBuy || + isQuoteLoading || + isRefreshing || + (isBuy ? !buyToken : !sellToken) + } + />src/apps/pulse/components/Sell/Sell.tsx (2)
1-1: Fix: stop shadowing global parseInt; use Number.parseInt and drop lodash import.Blocks Biome and is unnecessary.
Apply:
-import { parseInt } from 'lodash'; @@ - const percentage = isMax ? 100 : parseInt(item); + const percentage = isMax ? 100 : Number.parseInt(item, 10);Also applies to: 305-305
93-106: Balance lookup can mis-match on duplicate symbols—match by address+chain across all assets.Avoid symbol-based pre-filter to prevent wrong balances.
- // 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}` - ); - return contractBalance?.balance || 0; + // Find the contract balance by exact address+chain across all assets + const contractBalance = walletPortfolioData.result.data.assets + .flatMap((asset) => asset.contracts_balances || []) + .find( + (contract) => + contract?.address?.toLowerCase() === token.address.toLowerCase() && + contract?.chainId === `evm:${token.chainId}` + ); + return contractBalance?.balance ?? 0;src/apps/pulse/hooks/useRelaySell.ts (3)
1-3: Use initialized Relay client instance instead of global getClient().Avoids races and undefined client states.
-import { Execute, getClient } from '@relayprotocol/relay-sdk'; +import { Execute } from '@relayprotocol/relay-sdk'; @@ - const { isInitialized, accountAddress } = useRelaySdk(); + const { isInitialized, accountAddress, relayClient } = useRelaySdk(); @@ - // Get quote from Relay SDK - const client = getClient(); - const quote = await client.actions.getQuote(quoteRequest); + // Get quote from Relay SDK + if (!relayClient) { + setError('Relay SDK is not initialized. Please try again.'); + return null; + } + const quote = await relayClient.actions.getQuote(quoteRequest); @@ - [isInitialized, accountAddress] + [isInitialized, accountAddress, relayClient]Also applies to: 55-55, 193-196, 246-249
271-281: Avoid duplicate wrapping when the route already contains a wrap.Double-wrapping breaks execution; detect wrap in steps and skip manual wrap.
- const isWrapRequired = + let isWrapRequired = isWrappedToken( sellOffer.offer.details?.currencyIn?.currency?.address || '', token.chainId - ) && !isWrappedToken(token.address, token.chainId); + ) && !isWrappedToken(token.address, token.chainId); + + // Detect an existing wrap in the route + const wrappedTokenAddressForDetect = getWrappedTokenAddressIfNative( + token.address, + token.chainId + ); + const hasWrapInRoute = + steps.some((s) => s.id === 'wrap') || + steps.some((s) => + s.items?.some( + (it) => + it.data?.to?.toLowerCase() === + wrappedTokenAddressForDetect?.toLowerCase() && + (it.data?.value ?? '0') !== '0' + ) + ); + isWrapRequired = isWrapRequired && !hasWrapInRoute;Also applies to: 395-428
4-10: Validate fee receiver address (format and non-zero).Prevents misconfiguration leading to failed transfers.
import { createPublicClient, encodeFunctionData, erc20Abi, http, parseUnits, + isAddress, } from 'viem'; @@ - if (!feeReceiver) { - throw new Error('Fee receiver address is not configured'); - } + if (!feeReceiver) { + throw new Error('Fee receiver address is not configured'); + } + if (!isAddress(feeReceiver) || isZeroAddress(feeReceiver)) { + throw new Error('Invalid fee receiver address configured'); + }Also applies to: 288-294
🧹 Nitpick comments (11)
package.json (1)
161-169: Avoid mixing npm "overrides" with Yarn "resolutions". Pick one PM.Using both complicates resolution/debugging. If on npm/pnpm, drop "resolutions"; if on Yarn, drop "overrides" for viem and keep "resolutions".
src/apps/pulse/hooks/useTokenSearch.ts (1)
13-21: Reset debouncedSearchText on Sell to prevent stale fetches.Keeps skip logic simple and prevents lingering results when switching modes.
useEffect((): (() => void) | undefined => { if (isBuy) { const handler = setTimeout(() => { setDebouncedSearchText(searchText); }, 1000); return () => clearTimeout(handler); } - return undefined; + // Clear any pending search when not buying + setDebouncedSearchText(''); + return undefined; }, [searchText, isBuy]);src/apps/pulse/components/App/AppWrapper.tsx (2)
62-65: Standardize walletPortfolioData shape passed to children.Here you pass result.data (array) to Search, but HomeScreen passes the raw query result to its children. Pick one shape to avoid prop-type drift.
Would you like a quick patch to pass result.data consistently from HomeScreen as well?
21-35: Duplicate useGetWalletPortfolioQuery in AppWrapper and HomeScreen. Both components invoke the hook with identical args, creating two subscriptions and extra mental overhead. Consolidate the query—either call it once in AppWrapper and pass the data (and refetch/loading/error) into HomeScreen, or remove it from AppWrapper and let HomeScreen own it.src/apps/pulse/components/App/HomeScreen.tsx (3)
230-249: Unify walletPortfolioData prop shape to match AppWrapper/Search usage.Pass the same result.data array here to keep consumers consistent.
- <Buy + <Buy setSearching={setSearching} token={buyToken} - walletPortfolioData={walletPortfolioData} + walletPortfolioData={walletPortfolioData?.result?.data} payingTokens={payingTokens} setPreviewBuy={setPreviewBuy} setPayingTokens={setPayingTokens} setExpressIntentResponse={setExpressIntentResponse} /> @@ - <Sell + <Sell setSearching={setSearching} token={sellToken} - walletPortfolioData={walletPortfolioData} + walletPortfolioData={walletPortfolioData?.result?.data} setPreviewSell={setPreviewSell} setSellOffer={setSellOffer} setTokenAmount={setTokenAmount} />
70-76: Consider removing the local portfolio query and relying on parent-provided data.AppWrapper already fetches portfolio and exposes refetch; using one source avoids duplicate wiring.
1-3: Type-only import for SDK types.Use import type to avoid accidental runtime import from deep path.
-import { ExpressIntentResponse } from '@etherspot/intent-sdk/dist/cjs/sdk/types/user-intent-types'; +import type { ExpressIntentResponse } from '@etherspot/intent-sdk/dist/cjs/sdk/types/user-intent-types';src/apps/pulse/components/Sell/Sell.tsx (3)
85-87: Include getBestSellOffer in deps; remove eslint-disable to avoid stale closure.Prevents using an outdated SDK function reference.
- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [debouncedTokenAmount, token, isInitialized, notEnoughLiquidity]); + }, [debouncedTokenAmount, token, isInitialized, notEnoughLiquidity, getBestSellOffer]);
287-296: Guard against NaN usdValue in balance display.Prevent “NaN” in UI when price missing.
- {limitDigitsNumber(tokenBalance * parseFloat(token.usdValue))} + {limitDigitsNumber( + tokenBalance * + (Number.isFinite(Number(token.usdValue)) + ? Number(token.usdValue) + : 0) + )}
115-129: Allow gradual decimal input (e.g., “.”, “1.”) while keeping validation.Current logic blocks typical typing patterns.
- const input = e.target.value; - if (!input || !Number.isNaN(parseFloat(input))) { + const input = e.target.value; + const isValid = input === '' || /^[0-9]*\.?[0-9]*$/.test(input); + if (isValid) { setInputPlaceholder('0.00'); setTokenAmount(input); setParentTokenAmount(input); - if (input && token) { - const inputAmount = parseFloat(input); - setNotEnoughLiquidity(inputAmount > tokenBalance); - } else { - setNotEnoughLiquidity(false); - } + const inputAmount = parseFloat(input); + setNotEnoughLiquidity( + !!(token && input && !Number.isNaN(inputAmount) && inputAmount > tokenBalance) + ); }src/apps/pulse/hooks/useRelaySell.ts (1)
92-110: Optional: cache viem PublicClients per chain for allowance checks.Reduces client instantiation per call.
I can provide a small memoized Map<chainId, PublicClient> if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/LeftColumnTokenMarketDataRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensWithMarketDataTile/test/__snapshots__/TokensWithMarketDataTile.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
package.json(3 hunks)src/apps/pulse/components/App/AppWrapper.tsx(3 hunks)src/apps/pulse/components/App/HomeScreen.tsx(1 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(1 hunks)src/apps/pulse/components/Sell/Sell.tsx(2 hunks)src/apps/pulse/contexts/RefreshContext.tsx(1 hunks)src/apps/pulse/hooks/useRelaySdk.ts(1 hunks)src/apps/pulse/hooks/useRelaySell.ts(1 hunks)src/apps/pulse/hooks/useTokenSearch.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apps/pulse/components/Sell/PreviewSell.tsx
- src/apps/pulse/contexts/RefreshContext.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 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.611Z
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-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/hooks/useTokenSearch.tssrc/apps/pulse/components/App/AppWrapper.tsxsrc/apps/pulse/components/Sell/Sell.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/hooks/useRelaySell.ts
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/pulse/components/App/AppWrapper.tsx
📚 Learning: 2025-09-09T12:40:15.611Z
Learnt from: RanaBug
PR: pillarwallet/x#391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.611Z
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/App/HomeScreen.tsxsrc/apps/pulse/components/Sell/Sell.tsx
🧬 Code graph analysis (4)
src/apps/pulse/hooks/useRelaySell.ts (9)
src/apps/pulse/hooks/useRelaySdk.ts (1)
useRelaySdk(21-63)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/pulse/contexts/LoadingContext.tsx (1)
useLoading(34-40)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(60-79)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(1-9)src/apps/the-exchange/utils/wrappedTokens.ts (3)
getWrappedTokenAddressIfNative(33-44)isWrappedToken(23-28)isNativeToken(30-31)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
Token(19-29)src/apps/the-exchange/utils/blockchain.ts (2)
getNativeBalanceFromPortfolio(129-141)toWei(124-126)
src/apps/pulse/components/App/AppWrapper.tsx (4)
src/apps/pulse/contexts/LoadingContext.tsx (1)
LoadingProvider(15-32)src/apps/pulse/contexts/RefreshContext.tsx (1)
RefreshProvider(24-70)src/apps/pulse/components/Search/Search.tsx (1)
Search(57-381)src/apps/pulse/components/App/HomeScreen.tsx (1)
HomeScreen(36-263)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/contexts/RefreshContext.tsx (1)
useRefresh(72-78)src/apps/pulse/contexts/LoadingContext.tsx (1)
useLoading(34-40)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(41-44)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-34)
src/apps/pulse/components/Sell/Sell.tsx (6)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/types/api.ts (1)
WalletPortfolioMobulaResponse(750-752)src/apps/pulse/hooks/useRelaySell.ts (2)
SellOffer(41-44)useRelaySell(54-922)src/apps/pulse/contexts/RefreshContext.tsx (1)
useRefresh(72-78)src/utils/blockchain.ts (1)
getLogoForChainId(158-192)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
🪛 Biome (2.1.2)
src/apps/pulse/components/Sell/Sell.tsx
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (3)
package.json (1)
96-96: Viem v2.37.1 satisfies wagmi and Relay SDK peer ranges
Viem 2.37.1 falls within wagmi@2.14.16’sviem: 2.xrequirement and @relayprotocol/relay-sdk@2.4.2’sviem: >=2.26.0.src/apps/pulse/components/Sell/Sell.tsx (2)
131-140: Debounce fix looks good—clears offers when input clears.
142-154: Liquidity re-check on token/balance/amount change is sound.Matches the team’s behavior of resetting amount on token switch; this keeps state consistent.
| // Calculate fee distribution: 99% to user, 1% to fee receiver | ||
| // We swap 100% of the token, then take 1% of the received USDC as fee | ||
| // Use the actual USDC received from the swap (before 99% display calculation) | ||
| // The amount is already in wei format (6 decimals for USDC) | ||
| // Note: We use the quote amount as an estimate, but the actual amount may vary due to slippage | ||
| const actualUsdcReceived = BigInt( | ||
| sellOffer.offer.details?.currencyOut?.amount || '0' | ||
| ); | ||
| const usdcFeeAmount = (actualUsdcReceived * BigInt(1)) / BigInt(100); // 1% to fee receiver | ||
| const usdcUserAmount = actualUsdcReceived - usdcFeeAmount; // 99% to user | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm semantics of quote.details.currencyOut.amount before hard-coding 1% transfer.
If amount is not a guaranteed min-out, fee transfer may overdraw after slippage and fail.
Would you verify with Relay docs whether currencyOut.amount is minOut after slippageTolerance? If not, we should base the fee on the guaranteed min-out or adjust strategy.
Also applies to: 712-746
🌐 Web query:
Relay SDK getQuote response: is details.currencyOut.amount a guaranteed minimum output (respecting slippageTolerance) or an expected amount?
💡 Result:
Short answer: details.currencyOut.amount is the expected/output estimate; the guaranteed minimum (respecting slippageTolerance) is returned as minimumAmount (or documented “minimumAmount”) in the quote. See the Relay API/SDK quote response which includes both amount (estimated) and minimumAmount (minimum output after slippage). [1][2]
Sources:
- Relay getQuote / API reference (response shows amount and minimumAmount for route outputCurrency). [1]
- Relay swapping guide (tradeType and slippageTolerance behavior / expected vs exact output). [2]
Calculate fees using the guaranteed minimum output
Relay’s quote response provides currencyOut.amount as an estimate and minimumAmount as the slippage-protected minimum; use details.minimumAmount for usdcFeeAmount to avoid overdrawing when slippage occurs.
Applies to src/apps/pulse/hooks/useRelaySell.ts lines 301–311 (and similarly 712–746).
🤖 Prompt for AI Agents
In src/apps/pulse/hooks/useRelaySell.ts around lines 301 to 311, the fee
calculation currently uses sellOffer.offer.details.currencyOut.amount (an
estimate) which can overcharge the user if slippage occurs; replace that with
the slippage-protected sellOffer.offer.details.minimumAmount when computing
usdcFeeAmount and usdcUserAmount, falling back to currencyOut.amount if
minimumAmount is missing, and ensure values are parsed as BigInt before
arithmetic; apply the same change to the similar block at lines ~712–746 so both
fee calculations use minimumAmount with a safe fallback.
There was a problem hiding this comment.
@RanaBug Great! Thank you for fixing the fee calculation to use the guaranteed minimum output. This ensures the USDC fee transfer won't fail due to slippage during the swap execution.
🐰
╭─────╮
│ ✨ │
╰─────╯
\ o /
|/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| <div className="flex flex-col items-center justify-center min-h-screen bg-black"> | ||
| <div className="text-white text-lg">Invalid data</div> | ||
| <div className="text-white text-lg"> | ||
| No offer was found. Please check the token and the inpu amount and try |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/apps/pulse/hooks/useRelaySell.ts (3)
2-2: Use the initialized Relay client from useRelaySdk; avoid global getClient().Relying on getClient() can race SDK initialization and break in multi-account contexts. Use relayClient from useRelaySdk and guard for availability.
- import { Execute, getClient } from '@relayprotocol/relay-sdk'; + import { Execute } from '@relayprotocol/relay-sdk'; @@ - const { isInitialized, accountAddress } = useRelaySdk(); + const { isInitialized, accountAddress, relayClient } = useRelaySdk(); @@ - if (!isInitialized) { + if (!isInitialized || !relayClient) { setError('Unable to get quote. Please try again.'); return null; } @@ - // Get quote from Relay SDK - const client = getClient(); - const quote = await client.actions.getQuote(quoteRequest); + // Get quote from Relay SDK + const quote = await relayClient.actions.getQuote(quoteRequest); @@ - [isInitialized, accountAddress] + [isInitialized, accountAddress, relayClient]Also applies to: 55-55, 142-146, 193-196, 248-249
275-281: Avoid duplicate wrapping when the route already includes a wrap.If Relay already inserts a wrap step, adding a manual deposit() doubles value and can fail execution. Detect wrap in steps and skip the manual wrap.
- const isWrapRequired = + let isWrapRequired = isWrappedToken( sellOffer.offer.details?.currencyIn?.currency?.address || '', token.chainId ) && !isWrappedToken(token.address, token.chainId); + + // Detect wrap in route (step id or payable deposit to wrapped token) + const wrappedTokenAddressForDetect = getWrappedTokenAddressIfNative( + token.address, + token.chainId + ); + const hasWrapInRoute = + steps.some((s) => s.id === 'wrap') || + steps.some((s) => + s.items?.some( + (it) => + it.data?.to?.toLowerCase() === + wrappedTokenAddressForDetect?.toLowerCase() && + (it.data?.value ?? '0') !== '0' + ) + ); + isWrapRequired = isWrapRequired && !hasWrapInRoute;
4-10: Validate fee receiver address format (EVM address, non-zero, not 0xeeee...).Presence check alone is insufficient; invalid addresses will revert the transfer at the end of the batch.
import { createPublicClient, encodeFunctionData, erc20Abi, http, parseUnits, + isAddress, } from 'viem'; @@ const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; // Validate fee receiver address if (!feeReceiver) { throw new Error('Fee receiver address is not configured'); } + if (!isAddress(feeReceiver) || isZeroAddress(feeReceiver)) { + throw new Error('Invalid fee receiver address configured'); + } + // Disallow native-token sentinel address sometimes used elsewhere + if (/^0x[eE]{40}$/.test(feeReceiver)) { + throw new Error('Invalid fee receiver address configured (0xeeee… not allowed)'); + }Also applies to: 288-294
🧹 Nitpick comments (3)
src/apps/pulse/hooks/useRelaySell.ts (3)
486-514: Use erc20Abi for approve encoding.No need to inline the ABI; reduces risk of ABI typos and keeps consistency.
- const calldata = encodeFunctionData({ - abi: [ - { - inputs: [ - { internalType: 'address', name: 'spender', type: 'address' }, - { internalType: 'uint256', name: 'value', type: 'uint256' }, - ], - name: 'approve', - outputs: [{ internalType: 'bool', name: '', type: 'bool' }], - stateMutability: 'nonpayable', - type: 'function', - }, - ], - functionName: 'approve', - args: [spenderAddress as `0x${string}`, fromAmountBigInt], - }); + const calldata = encodeFunctionData({ + abi: erc20Abi, + functionName: 'approve', + args: [spenderAddress as `0x${string}`, fromAmountBigInt], + });Apply the same change to the ERC20 (non-wrapped) approval block below.
Also applies to: 569-597
92-110: Cache viem public clients per chain to reduce overhead.isAllowanceSet recreates a client on every call. Memoize one per chainId within the hook.
Outline:
- Keep a Map<number, PublicClient> in a ref.
- Create once per chainId; reuse for subsequent allowance reads.
140-141: Clamp slippage to sane bounds.Guard against accidental large values; e.g., 1–500 bps.
- slippage = 0.03, + slippage = 0.03, @@ - slippageTolerance: Math.floor(slippage * 10000), // Convert to basis points + slippageTolerance: Math.min(5000, Math.max(1, Math.floor(slippage * 10000))),Also applies to: 189-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/pulse/components/Sell/PreviewSell.tsx(1 hunks)src/apps/pulse/hooks/useRelaySdk.ts(1 hunks)src/apps/pulse/hooks/useRelaySell.ts(1 hunks)src/apps/pulse/hooks/useTokenSearch.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/apps/pulse/hooks/useTokenSearch.ts
- src/apps/pulse/hooks/useRelaySdk.ts
- src/apps/pulse/components/Sell/PreviewSell.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 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.611Z
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-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/hooks/useRelaySell.ts
🧬 Code graph analysis (1)
src/apps/pulse/hooks/useRelaySell.ts (9)
src/apps/pulse/hooks/useRelaySdk.ts (1)
useRelaySdk(21-64)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/pulse/contexts/LoadingContext.tsx (1)
useLoading(34-40)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(60-79)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(1-9)src/apps/the-exchange/utils/wrappedTokens.ts (3)
getWrappedTokenAddressIfNative(33-44)isWrappedToken(23-28)isNativeToken(30-31)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
Token(19-29)src/apps/the-exchange/utils/blockchain.ts (2)
getNativeBalanceFromPortfolio(129-141)toWei(124-126)
🔇 Additional comments (1)
src/apps/pulse/hooks/useRelaySell.ts (1)
450-469: Decode ERC-20 approval tx to extract the spender from its ABI-encoded data
Relay steps/items don’t include an explicitallowanceTargetorspenderfield—approval calls are generic transactions wheretois the token contract, and the actual spender is the first argument of theapprove(spender, amount)call withindata. At src/apps/pulse/hooks/useRelaySell.ts lines 450–469, replace theitem.data.toheuristic with an ABI decode ofitem.data.data(ERC-20approvesignature) to retrieve the spender address.Likely an incorrect or invalid review comment.
2 participants
Description
--> Relay SDK implementation
--> Fee implementation
--> UI implementation and improvements
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
UI/Style
Infrastructure