Fixes after QA review on feat/PRO-3638/sell-feature-pulse#395
Fixes after QA review on feat/PRO-3638/sell-feature-pulse#395
Conversation
Deploying x with
|
| Latest commit: |
24f93a6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://156998f2.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3638-sell-feature-p.x-e62.pages.dev |
WalkthroughThis PR removes global Loading/Refresh contexts, refactors sell-flow components to manage refresh locally via useRelaySell, simplifies AppWrapper routing, enhances Search with ESC/click-outside close, standardizes Esc’s onClose prop, updates SellButton text rendering, and adjusts tests to new providers and APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant HomeScreen
participant useRelaySell
participant PreviewSell
participant Sell
User->>HomeScreen: Pull to refresh / refresh action
HomeScreen->>HomeScreen: set isRefreshingHome = true
HomeScreen->>useRelaySell: walletPortfolio.refetch()
alt sellToken && tokenAmount && isInitialized
HomeScreen->>useRelaySell: getBestSellOffer(sellToken, amount)
useRelaySell-->>HomeScreen: offer | error
HomeScreen->>PreviewSell: onSellOfferUpdate(offer|null)
end
HomeScreen->>Sell: isRefreshing = isRefreshingHome
HomeScreen->>HomeScreen: set isRefreshingHome = false
sequenceDiagram
autonumber
actor User
participant Search
participant Esc
User->>Search: Open modal
Search->>Search: attach click-outside + ESC listeners
alt Click outside modal
User->>Search: click document
Search->>Search: handleClose() reset state + close
else Press ESC
User->>Esc: keydown Escape
Esc-->>Search: onClose()
Search->>Search: handleClose() reset state + close
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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.
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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/Sell/Sell.tsx (1)
92-106: Fix balance lookup: symbol-first search can miss the correct contractIf multiple assets share the same symbol, finding by symbol first can return the wrong asset and hide the actual balance. Search contracts across all assets by address+chain instead.
Apply:
- // 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 for the specific token address and chain across all assets + const assets = walletPortfolioData.result.data.assets || []; + for (const asset of assets) { + const contractBalance = asset.contracts_balances?.find( + (contract) => + contract.address?.toLowerCase() === token.address.toLowerCase() && + contract.chainId === `evm:${token.chainId}` + ); + if (contractBalance) return contractBalance.balance || 0; + } + return 0;
🧹 Nitpick comments (23)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
144-144: Snapshot without wrappers is fine; consider focusing on behavior over structure.To reduce snapshot churn from minor markup changes, prefer targeted assertions for key UI states.
src/apps/pulse/components/Misc/Esc.tsx (2)
10-21: ESC key handler looks solid; minor a11y/compat tweak.Consider accepting legacy "Esc" and declaring a keyboard shortcut for assistive tech.
Apply:
- const handleKeyDown = (event: KeyboardEvent) => { - if (event.key === 'Escape') { + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape' || event.key === 'Esc') { onClose(); } };
23-30: Nice: clear, accessible button API.aria-label and data-testid are in place; onClick is correctly wired to onClose.
You can also hint the shortcut to AT:
<button className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px]" onClick={onClose} type="button" aria-label="Close" + aria-keyshortcuts="Escape" data-testid="esc-button"src/apps/pulse/hooks/useRelaySell.ts (2)
724-731: Clarify step description for fee transfer.Slightly clearer wording improves activity logs without changing behavior.
- stepDescription: 'USDC Fee Transfer', + stepDescription: 'Transfer USDC fee (1%)',
858-864: Tighten transaction description copy.Current phrasing is a bit long and mixes step detail inline. Consider a concise, consistent template.
- const description = `Selling ${amount} ${token.symbol} for ${sellOffer.tokenAmountToReceive.toFixed(2)} USDC on ${stepDescription}`; + const description = `Sell ${amount} ${token.symbol} → ~${sellOffer.tokenAmountToReceive.toFixed(2)} USDC (${stepDescription})`;src/apps/pulse/components/App/tests/HomeScreen.test.tsx (3)
74-79: Avoid duplicating wrapper setup in snapshot.Reuse renderWithProviders or create a helper for renderer.create to keep tests DRY.
120-125: Simplify rerender: wrapper is preserved.When using renderWithProviders, you can rerender with just the element.
- rerender( - <GlobalTransactionsBatchProvider> - <BottomMenuModalProvider> - <HomeScreen {...mockProps} isBuy={false} /> - </BottomMenuModalProvider> - </GlobalTransactionsBatchProvider> - ); + rerender(<HomeScreen {...mockProps} isBuy={false} />);
236-241: Same here: use preserved wrapper on rerender.- rerender( - <GlobalTransactionsBatchProvider> - <BottomMenuModalProvider> - <HomeScreen {...mockProps} isBuy={false} /> - </BottomMenuModalProvider> - </GlobalTransactionsBatchProvider> - ); + rerender(<HomeScreen {...mockProps} isBuy={false} />);src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
113-113: Snapshot aligns with new rendering approach.Looks good as a baseline; keep behavioral tests as primary signal.
src/apps/pulse/components/Sell/tests/Sell.test.tsx (1)
89-90: Expose mocks for assertionsReturning
mockUseRelaySellis fine. Consider also returning its fns (e.g.,getBestSellOffer) so individual tests can assert call args directly if needed.src/apps/pulse/components/App/AppWrapper.tsx (1)
48-69: Minor: avoid extra effect re-runs from URLSearchParamsUnrelated to this hunk but nearby:
useQuery()creates a freshURLSearchParamseach render; the effect at Line 40 will re-run more than needed. Consider derivingtokenAddressviauseLocation().search+useMemo/useSearchParamsto stabilize deps.src/apps/pulse/components/App/tests/AppWrapper.test.tsx (3)
40-48: Centralized render helper is good; consider MemoryRouter in the wrapper
renderWithProvidersis solid. To reduce repetition, you could acceptinitialEntriesand includeMemoryRouterinside the wrapper.
90-98: Use the same wrapper in the snapshot testSnapshot currently reimplements the provider tree inline. Prefer
renderWithProviders(<MemoryRouter>...)for consistency.
258-262: Simplify rerender pathYou can call the returned
rerenderwith the new element only; re-wrapping with providers isn’t necessary since the wrapper is already fixed byrenderWithProviders.Also applies to: 273-281
src/apps/pulse/components/Sell/SellButton.tsx (1)
100-110: Compute disabled once to avoid repeated workCache
isDisabled()to avoid triple eval and pass it through togetButtonText.Apply:
const isDisabled = () => { return ( isLoadingOffer || !token || !(parseFloat(tokenAmount) > 0) || !sellOffer || sellOffer.tokenAmountToReceive === 0 || notEnoughLiquidity || !isInitialized ); }; return ( <button className="flex-1 items-center justify-center" onClick={handleSellClick} - disabled={isDisabled()} + disabled={disabled} style={{ - backgroundColor: isDisabled() ? '#29292F' : '#8A77FF', - color: isDisabled() ? 'grey' : '#FFFFFF', + backgroundColor: disabled ? '#29292F' : '#8A77FF', + color: disabled ? 'grey' : '#FFFFFF', borderRadius: 8, }} type="button" > {getButtonText( isLoadingOffer, isInitialized, token, tokenAmount, - sellOffer, - isDisabled() + sellOffer, + disabled )} </button> );Add above the return:
+ const disabled = isDisabled();Also applies to: 112-131
src/apps/pulse/components/Search/Search.tsx (3)
127-137: Stabilize handleClose to avoid stale closure issuesWrap
handleCloseinuseCallbackso click-outside/ESC listeners always use the latestisBuyand routing state.Apply:
- const handleClose = () => { + const handleClose = useCallback(() => { setSearchText(''); // It resets search type to MyHoldings if on sell screen if (!isBuy) { setSearchType(SearchType.MyHoldings); } else { setSearchType(undefined); } setSearching(false); removeQueryParams(); - }; + }, [isBuy, setSearchText, setSearching, removeQueryParams]);Add import (outside this hunk):
import React, { Dispatch, SetStateAction, useEffect, useRef, useState, useCallback } from 'react';
139-156: Click-outside effect should depend on handleCloseThis removes the ESLint disable and ensures correct behavior if props change.
Apply:
- useEffect(() => { + useEffect(() => { const handleClickOutside = (event: MouseEvent) => { if ( searchModalRef.current && !searchModalRef.current.contains(event.target as Node) ) { handleClose(); } }; document.addEventListener('mousedown', handleClickOutside); return () => { document.removeEventListener('mousedown', handleClickOutside); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [handleClose]);
157-170: ESC key effect should depend on handleCloseSame rationale as above; remove the disable and add the dependency.
Apply:
- useEffect(() => { + useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Escape') { handleClose(); } }; document.addEventListener('keydown', handleKeyDown); return () => { document.removeEventListener('keydown', handleKeyDown); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [handleClose]);src/apps/pulse/components/Sell/Sell.tsx (4)
229-242: Improve number input UX and validationUse inputMode/pattern to surface numeric keyboards and prevent invalid strings (e.g., “1..2”) that later crash parseUnits downstream.
<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="^\d*\.?\d*$" + autoComplete="off" onFocus={() => setInputPlaceholder('')} data-testid="pulse-sell-amount-input" />
244-263: Tooltip: add a11y + mobile supportMouse-only tooltip isn’t available on touch and lacks semantics. Add title/aria attributes and role=tooltip. Consider showing full symbol on focus.
- <p - className="text-grey ml-0 flex-shrink-0 opacity-50 cursor-help text-4xl font-medium" - data-testid="pulse-sell-token-symbol" - onMouseEnter={() => setShowTooltip(true)} - onMouseLeave={() => setShowTooltip(false)} - > + <p + className="text-grey ml-0 flex-shrink-0 opacity-50 cursor-help text-4xl font-medium" + data-testid="pulse-sell-token-symbol" + onMouseEnter={() => setShowTooltip(true)} + onMouseLeave={() => setShowTooltip(false)} + onFocus={() => setShowTooltip(true)} + onBlur={() => setShowTooltip(false)} + title={token.symbol} + aria-describedby="sell-token-symbol-tooltip" + tabIndex={0} + > {token.symbol.length > 4 ? `${token.symbol.slice(0, 3)}...` : token.symbol} </p> - {showTooltip && token.symbol.length > 4 && ( - <div className="absolute bottom-full left-1/2 transform -translate-x-1/2 mb-2 px-2 py-1 bg-black text-white text-xs rounded shadow-lg z-10 whitespace-nowrap"> + {showTooltip && token.symbol.length > 4 && ( + <div + id="sell-token-symbol-tooltip" + role="tooltip" + className="absolute bottom-full left-1/2 transform -translate-x-1/2 mb-2 px-2 py-1 bg-black text-white text-xs rounded shadow-lg z-10 whitespace-nowrap" + > {token.symbol} </div> )}
307-313: Drop lodash.parseInt import; use Number.parseInt with radixAvoid an extra dependency for this simple case and be explicit with radix.
- {['10%', '25%', '50%', '75%', 'MAX'].map((item) => { + {['10%', '25%', '50%', '75%', 'MAX'].map((item) => { const isMax = item === 'MAX'; - const percentage = isMax ? 100 : parseInt(item); + const percentage = isMax ? 100 : Number.parseInt(item, 10);And at file top:
-import { parseInt } from 'lodash';
141-154: Confirm: should amount reset on token change (per prior behavior)?Previous behavior reset the input when switching tokens to avoid stale liquidity states. Current code recalculates liquidity but keeps the amount. Confirm intended UX or re-introduce a reset on token change.
Possible re-introduction:
+ useEffect(() => { + setTokenAmount(''); + setParentTokenAmount(''); + setLocalSellOffer(null); + setNotEnoughLiquidity(false); + }, [token]);src/apps/pulse/components/Sell/PreviewSell.tsx (1)
108-137: Preview refresh flow is sound; minor guard improvementFlow and guards are good. Tiny nit: early-return when prerequisites fail avoids setting/clearing the spinner unnecessarily.
- const refreshPreviewSellData = useCallback(async () => { - setIsRefreshingPreview(true); - try { - // Only fetch new sell offer - wallet portfolio is already fresh from HomeScreen - if (sellToken && tokenAmount && isInitialized && onSellOfferUpdate) { + const refreshPreviewSellData = useCallback(async () => { + if (!sellToken || !tokenAmount || !isInitialized || !onSellOfferUpdate) return; + setIsRefreshingPreview(true); + try { const newOffer = await getBestSellOffer({ fromAmount: tokenAmount, fromTokenAddress: sellToken.address, fromChainId: sellToken.chainId, fromTokenDecimals: sellToken.decimals, }); onSellOfferUpdate(newOffer); } catch (e) { console.error('Failed to refresh sell offer:', e); - if (onSellOfferUpdate) { - onSellOfferUpdate(null); - } + onSellOfferUpdate(null); } finally { setIsRefreshingPreview(false); } }, [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/apps/pulse/assets/usd-coin-usdc-logo.pngis excluded by!**/*.pngsrc/apps/pulse/components/Buy/tests/__snapshots__/PreviewBuy.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/PreviewSell.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/Sell.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/SellButton.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
src/apps/pulse/components/App/AppWrapper.tsx(1 hunks)src/apps/pulse/components/App/HomeScreen.tsx(5 hunks)src/apps/pulse/components/App/tests/AppWrapper.test.tsx(12 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(4 hunks)src/apps/pulse/components/Buy/PreviewBuy.tsx(1 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(2 hunks)src/apps/pulse/components/Misc/Esc.tsx(1 hunks)src/apps/pulse/components/Search/Search.tsx(9 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(8 hunks)src/apps/pulse/components/Sell/Sell.tsx(4 hunks)src/apps/pulse/components/Sell/SellButton.tsx(3 hunks)src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx(2 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(2 hunks)src/apps/pulse/components/Sell/tests/SellButton.test.tsx(1 hunks)src/apps/pulse/components/Status/IntentTracker.tsx(2 hunks)src/apps/pulse/contexts/LoadingContext.tsx(0 hunks)src/apps/pulse/contexts/RefreshContext.tsx(0 hunks)src/apps/pulse/hooks/tests/useRelaySell.test.tsx(0 hunks)src/apps/pulse/hooks/useRelaySell.ts(2 hunks)
💤 Files with no reviewable changes (3)
- src/apps/pulse/hooks/tests/useRelaySell.test.tsx
- src/apps/pulse/contexts/LoadingContext.tsx
- src/apps/pulse/contexts/RefreshContext.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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/tests/HomeScreen.test.tsxsrc/apps/pulse/components/Sell/tests/PreviewSell.test.tsxsrc/apps/pulse/components/App/AppWrapper.tsxsrc/apps/pulse/components/App/tests/AppWrapper.test.tsxsrc/apps/pulse/hooks/useRelaySell.ts
📚 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/Sell/Sell.tsxsrc/apps/pulse/components/Sell/SellButton.tsxsrc/apps/pulse/components/Sell/PreviewSell.tsx
🧬 Code graph analysis (11)
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
HomeScreen(32-297)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
src/apps/pulse/components/Buy/Buy.tsx (1)
Buy(35-442)
src/apps/pulse/components/App/AppWrapper.tsx (2)
src/apps/pulse/components/Search/Search.tsx (1)
Search(59-425)src/apps/pulse/components/App/HomeScreen.tsx (1)
HomeScreen(32-297)
src/apps/pulse/components/Sell/Sell.tsx (1)
src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(38-41)
src/apps/pulse/components/Status/IntentTracker.tsx (1)
src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)
src/apps/pulse/components/App/tests/AppWrapper.test.tsx (1)
src/apps/pulse/components/App/AppWrapper.tsx (1)
AppWrapper(11-70)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)
src/apps/pulse/components/Search/Search.tsx (3)
src/utils/number.tsx (1)
limitDigitsNumber(42-69)src/apps/pulse/components/Search/ChainSelect.tsx (1)
ChainSelectButton(3-13)src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)
src/apps/pulse/components/Sell/SellButton.tsx (3)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(38-41)src/utils/number.tsx (1)
limitDigitsNumber(42-69)
src/apps/pulse/components/App/HomeScreen.tsx (1)
src/apps/pulse/hooks/useRelaySell.ts (2)
useRelaySell(51-918)SellOffer(38-41)
src/apps/pulse/components/Sell/PreviewSell.tsx (2)
src/apps/pulse/hooks/useRelaySell.ts (2)
SellOffer(38-41)useRelaySell(51-918)src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)
🔇 Additional comments (27)
src/apps/pulse/components/Buy/tests/Buy.test.tsx (1)
134-134: Good move: remove obsolete providers.Rendering Buy directly matches the removal of Loading/Refresh contexts. Tests stay focused on component behavior.
src/apps/pulse/components/Status/IntentTracker.tsx (2)
5-9: Imports aligned with Esc API change.Swapping to the new Esc and keeping TransactionStatus import intact makes sense.
138-138: Prop rename applied correctly.matches the new EscProps.
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
121-121: Prop rename applied correctly.is consistent with the updated Esc API.
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
40-45: Providers updated appropriately.Using GlobalTransactionsBatchProvider and BottomMenuModalProvider reflects the new app wiring.
src/apps/pulse/components/Sell/tests/SellButton.test.tsx (1)
79-85: Good: assert fragments, not a monolithic string.This reduces brittleness from formatting changes.
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
93-93: Direct render is appropriate.Removing obsolete providers keeps the test lean.
src/apps/pulse/components/Sell/tests/Sell.test.tsx (2)
92-94: Provider-less render is appropriateRendering
<Sell />without legacy Loading/Refresh providers matches the refactor and keeps tests lean.
112-114: Updated assertion is correctChecking that
pulse-sell-token-symbolis absent when no token is selected aligns with the new UI.src/apps/pulse/components/App/AppWrapper.tsx (1)
48-69: Simplified conditional render: good cleanupSwitching to a single ternary for Search vs HomeScreen removes unnecessary wrappers and matches the provider-light approach.
src/apps/pulse/components/App/tests/AppWrapper.test.tsx (3)
16-19: Providers imported at the right levelUsing
GlobalTransactionsBatchProviderandBottomMenuModalProviderhere mirrors app runtime and fixes missing context issues in tests.
106-110: Broad replacement with renderWithProviders looks goodAll these cases correctly switch to the shared helper, reducing boilerplate and drift across tests.
Also applies to: 125-130, 140-144, 153-157, 169-177, 197-205, 224-228, 243-247
36-38: Empty Redux store is fine; watch for future selector usageSince reducer is
{}, ensure components under test don’t rely on selectors. If they start doing so, switch to a minimal slice or a mock store with preloadedState.Would you like a tiny reducer scaffold added for tests to prevent accidental regressions?
src/apps/pulse/components/Sell/SellButton.tsx (3)
10-15: Imports for number formatting and HighDecimalsFormatted are appropriateThese keep rendering logic focused and consistent with app-wide formatting.
21-23: Helper signature extension is fineAdding
sellOfferandisDisabledkeeps the button text logic encapsulated.
37-65: Clear, richer CTA text when actionableThe inline UI (“Sell X TOKEN for Y USDC”) with digit limiting is a UX win and avoids string parsing in tests.
src/apps/pulse/components/Search/Search.tsx (2)
18-18: Modal close UX improvements look good
- Using
limitDigitsNumberfor price-derived values avoids noisy decimals.searchModalRef+ click-outside and theEscbutton integrate well with the new close behavior.Also applies to: 29-30, 87-87, 263-263, 298-319
206-211: Price formatting change is appropriateSwitching to
limitDigitsNumber(...).toString()forusdValueyields consistent, readable numbers across buy/sell selection.Also applies to: 215-223, 226-234, 236-246
src/apps/pulse/components/Sell/Sell.tsx (1)
34-36: Prop plumbing for refresh looks goodPassing external refresh via isRefreshing and folding it into isLoadingOffer is correct and keeps the button state consistent during global refresh.
Also applies to: 45-46, 362-364
src/apps/pulse/components/App/HomeScreen.tsx (3)
42-43: Good: consume useRelaySell directly for refresh logicImporting getBestSellOffer and isInitialized here simplifies the refresh flow and removes context coupling.
239-245: Refresh button state wiring looks consistentUsing isRefreshingHome for isLoading/disabled is coherent and prevents concurrent refresh.
273-281: Propagating isRefreshing to Sell is correctKeeps offer-loading UX consistent between manual quote fetch and global refresh.
src/apps/pulse/components/Sell/PreviewSell.tsx (5)
34-45: API swap to onSellOfferUpdate is clearThe new prop better communicates intent; wiring from HomeScreen via setSellOffer looks correct.
49-57: Localize refresh state; remove global contextsIntroducing isRefreshingPreview and using useRelaySell directly is the right decoupling.
231-234: Refresh button UX is correctSpinner and disabled states align with isRefreshingPreview.
311-317: USDC logo addition is fineStatic logo usage with chain badge looks correct.
351-365: Receiving amount loader toggle is correctClean conditional UI; no further action.
| const handleRefresh = useCallback(async () => { | ||
| await Promise.all([refetchWalletPortfolio(), refreshSell()]); | ||
| }, [refetchWalletPortfolio, refreshSell]); | ||
| setIsRefreshingHome(true); | ||
|
|
||
| try { | ||
| const refreshPromises: Promise<void>[] = [ | ||
| Promise.resolve(refetchWalletPortfolio()), | ||
| ]; | ||
|
|
||
| // If we have the required data, refresh the sell offer (regardless of whether one exists) | ||
| if (sellToken && tokenAmount && isInitialized) { | ||
| const sellOfferPromise = (async () => { | ||
| try { | ||
| const newOffer = await getBestSellOffer({ | ||
| fromAmount: tokenAmount, | ||
| fromTokenAddress: sellToken.address, | ||
| fromChainId: sellToken.chainId, | ||
| fromTokenDecimals: sellToken.decimals, | ||
| }); | ||
| setSellOffer(newOffer); | ||
| } catch (error) { | ||
| console.error('Failed to refresh sell offer:', error); | ||
| setSellOffer(null); | ||
| } | ||
| })(); | ||
| refreshPromises.push(sellOfferPromise); | ||
| } | ||
|
|
||
| await Promise.all(refreshPromises); | ||
| } catch (error) { | ||
| console.error('Refresh failed:', error); | ||
| } finally { | ||
| setIsRefreshingHome(false); | ||
| } | ||
| }, [ | ||
| refetchWalletPortfolio, | ||
| sellToken, | ||
| tokenAmount, | ||
| isInitialized, | ||
| getBestSellOffer, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make wallet refetch awaitable; current Promise.resolve may not wait for network
If refetchWalletPortfolio returns void, Promise.resolve won’t track the underlying refetch; isRefreshingHome may flip false too early. Prefer a Promise-returning refetch and await it.
-interface HomeScreenProps {
+interface HomeScreenProps {
...
- refetchWalletPortfolio: () => void;
+ refetchWalletPortfolio: () => Promise<void>;
}
const handleRefresh = useCallback(async () => {
setIsRefreshingHome(true);
try {
- const refreshPromises: Promise<void>[] = [
- Promise.resolve(refetchWalletPortfolio()),
- ];
+ const refreshPromises: Promise<void>[] = [refetchWalletPortfolio()];If changing the prop type isn’t feasible, consider using the local query’s refetch from useGetWalletPortfolioQuery instead.
🤖 Prompt for AI Agents
In src/apps/pulse/components/App/HomeScreen.tsx around lines 52 to 92, the
refresh uses Promise.resolve(refetchWalletPortfolio()) which won’t await an
underlying asynchronous refetch if refetchWalletPortfolio returns void; change
the code so the wallet refetch is a real Promise: either 1) update the
refetchWalletPortfolio prop to return a Promise<void> and call/await it directly
when building refreshPromises, or 2) if you cannot change the prop signature,
call the local query hook’s refetch (useGetWalletPortfolioQuery().refetch) which
returns a Promise and push/await that Promise in refreshPromises; ensure the
Promise you add represents the actual network operation so
setIsRefreshingHome(false) only runs after the real refetch completes.
💡 Verification agent
🧩 Analysis chain
Verify removal of legacy Loading/Refresh contexts across repo
Ensure no stale imports/usages remain.
🏁 Script executed:
#!/bin/bash
# Find any leftover context usages or old onRefresh props.
rg -nP -C2 '(LoadingContext|RefreshContext|useLoading|useRefresh|LoadingProvider|RefreshProvider|\bonRefresh\b)'Length of output: 401
Update remaining test using onRefresh
Search found only one leftover: src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx — onRefresh: vi.fn() (≈line 89). Remove or update this test to the new refresh API; no other LoadingContext/RefreshContext/useLoading/useRefresh/LoadingProvider/RefreshProvider occurrences found.
🤖 Prompt for AI Agents
In src/apps/pulse/components/App/HomeScreen.tsx lines 52–92 we switched to a
centralized refresh API (handleRefresh using refetchWalletPortfolio and
getBestSellOffer) so update the remaining test at
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (around line ~89) that
still passes onRefresh: vi.fn(); remove that obsolete onRefresh prop and instead
mock or spy the new refresh mechanism used by the component (e.g., the refresh
hook or RefreshContext/provider your app now uses), trigger the refresh via that
API in the test, and update assertions to expect the new refresh calls/behavior
rather than the old onRefresh mock.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit