PRO-3833-USDC directional changes for Sell sides#446
Conversation
Deploying x with
|
| Latest commit: |
922ae9e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36cf0b5d.x-e62.pages.dev |
| Branch Preview URL: | https://feature-pro-3833-usdc-direct.x-e62.pages.dev |
WalkthroughHomeScreen now reads wallet portfolio, computes per-chain stable balances to determine maxStableCoinBalance and selectedChainIdForSettlement, exposes a SettingsMenu for custom buy/sell presets and settlement chain, and threads settlement-chain and preset props into Buy, Sell, PreviewSell, relay, and gas-estimation flows. Changes
Sequence Diagram(s)sequenceDiagram
participant HS as HomeScreen
participant WP as WalletPortfolioQuery
participant SM as SettingsMenu
participant Buy as Buy
participant Sell as Sell
participant Relay as Relay (useRelaySell / useGasEstimation)
HS->>WP: fetch walletPortfolioData
WP-->>HS: walletPortfolioData
HS->>HS: compute per-chain stable balances → maxStableCoinBalance
HS->>HS: set selectedChainIdForSettlement
alt Open Settings
HS->>SM: render (customBuyAmounts, customSellAmounts, selectedChainId)
SM-->>HS: commit updated presets & selectedChainId
end
HS->>Buy: pass maxStableCoinBalance + customBuyAmounts
HS->>Sell: pass customSellAmounts + selectedChainIdForSettlement
Sell->>Relay: request offer / build txs (toChainId = selectedChainIdForSettlement)
Relay-->>Sell: sell offer / txs (targeting toChainId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/pulse/hooks/useGasEstimation.ts (1)
158-169: MissingtoChainIdin useEffect dependency array.The
useEffecttriggers gas estimation when dependencies change, but it's missingtoChainIdin its dependency array. SinceestimateGasFees(called viaestimateGasFeesRef.current()) depends ontoChainId(line 151), gas estimation won't re-run when the user changes the settlement chain.Apply this diff:
useEffect(() => { if ( sellOffer && sellToken && kit && tokenAmount && estimateGasFeesRef.current && !isPaused ) { estimateGasFeesRef.current(); } - }, [sellOffer, sellToken, kit, tokenAmount, isPaused]); + }, [sellOffer, sellToken, kit, tokenAmount, isPaused, toChainId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/apps/pulse/components/App/tests/__snapshots__/AppWrapper.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/App/tests/__snapshots__/HomeScreen.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Buy/tests/__snapshots__/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!**/*.snap
📒 Files selected for processing (14)
src/apps/pulse/components/App/HomeScreen.tsx(8 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(1 hunks)src/apps/pulse/components/Buy/Buy.tsx(6 hunks)src/apps/pulse/components/Buy/tests/Buy.test.tsx(3 hunks)src/apps/pulse/components/Misc/Esc.tsx(2 hunks)src/apps/pulse/components/Misc/Settings.tsx(1 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(8 hunks)src/apps/pulse/components/Sell/Sell.tsx(4 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/Settings/SettingsMenu.tsx(1 hunks)src/apps/pulse/hooks/tests/useRelaySell.test.tsx(4 hunks)src/apps/pulse/hooks/useGasEstimation.ts(3 hunks)src/apps/pulse/hooks/useRelaySell.ts(9 hunks)
🧰 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.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
PR: pillarwallet/x#391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Sell/tests/Sell.test.tsxsrc/apps/pulse/components/Sell/tests/PreviewSell.test.tsxsrc/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Sell/PreviewSell.tsxsrc/apps/pulse/components/Buy/Buy.tsxsrc/apps/pulse/components/App/HomeScreen.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/Sell/tests/PreviewSell.test.tsxsrc/apps/pulse/components/App/HomeScreen.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/components/Buy/Buy.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/HomeScreen.tsx
🧬 Code graph analysis (4)
src/apps/pulse/components/Settings/SettingsMenu.tsx (2)
src/utils/blockchain.ts (2)
CompatibleChains(321-323)getLogoForChainId(165-199)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)
src/apps/pulse/components/Sell/PreviewSell.tsx (1)
src/utils/blockchain.ts (1)
getLogoForChainId(165-199)
src/apps/pulse/components/Buy/Buy.tsx (1)
src/apps/pulse/utils/blockchain.ts (1)
isNativeToken(17-18)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
SettingsMenu(22-379)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)src/apps/pulse/components/Misc/Settings.tsx (1)
Settings(7-21)src/apps/pulse/components/Buy/Buy.tsx (1)
Buy(69-663)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (32)
src/apps/pulse/components/Misc/Esc.tsx (1)
2-2: LGTM! Clean icon migration.The replacement of text with an SVG icon improves UI consistency while maintaining accessibility and functionality.
Also applies to: 28-32
src/apps/pulse/hooks/tests/useRelaySell.test.tsx (1)
385-385: LGTM! Test coverage properly updated.The addition of
toChainIdto test calls correctly exercises the new settlement chain functionality, including edge cases with unsupported chains.Also applies to: 442-442, 512-512, 584-584
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
106-106: LGTM! Test updated for Settings button.The test assertion correctly reflects the UI change from "Save" to "Settings" button.
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
133-133: LGTM! Test properly updated for settlement chain.The test correctly includes the new
selectedChainIdForSettlementprop and passes it toexecuteSell, aligning with the production code changes.Also applies to: 247-247
src/apps/pulse/components/Misc/Settings.tsx (1)
3-5: LGTM! Settings component properly enhanced.The addition of the onClick handler, corrected aria-label, and test ID properly enables the Settings functionality while maintaining accessibility and testability.
Also applies to: 7-14
src/apps/pulse/components/Buy/tests/Buy.test.tsx (3)
198-202: LGTM! Test props properly extended.The addition of
maxStableCoinBalanceandcustomBuyAmountsto mockProps correctly supports the new Buy component functionality.
402-405: LGTM! Proper handling of missing data.Providing an empty
portfolioTokensarray when wallet portfolio data is undefined ensures consistent test behavior.
581-584: LGTM! Low balance scenario properly tested.The test correctly exercises the insufficient balance warning by setting
maxStableCoinBalancebelow the minimum required amount.src/apps/pulse/components/Sell/Sell.tsx (4)
49-50: LGTM! Props properly defined.The addition of
customSellAmountsandselectedChainIdForSettlementprops enables dynamic configuration of sell presets and settlement chain handling.
63-64: LGTM! Props properly destructured.The new props are correctly extracted from the props object for use in the component.
94-94: LGTM! Settlement chain properly propagated.Passing
selectedChainIdForSettlementastoChainIdtogetBestSellOfferensures sell quotes are calculated for the correct destination chain.
450-450: LGTM! Dynamic sell amounts enabled.Replacing hardcoded values with
customSellAmountsenables flexible configuration of quick-select amounts from parent components.src/apps/pulse/components/Sell/PreviewSell.tsx (6)
39-39: LGTM! Settlement chain prop added.The
selectedChainIdForSettlementprop is properly defined and destructured for use throughout the component.Also applies to: 51-51
84-84: LGTM! Gas estimation uses settlement chain.Passing
selectedChainIdForSettlementastoChainIdtouseGasEstimationensures gas fees are estimated for the correct destination chain.
216-216: LGTM! USDC address resolved on settlement chain.Using
selectedChainIdForSettlementto get the USDC address correctly targets the destination chain where funds will be received.
252-252: LGTM! Quote refresh uses settlement chain.The refresh logic correctly passes
selectedChainIdForSettlementto ensure updated quotes are for the correct destination chain.
369-374: LGTM! Settlement chain passed to executeSell.The execution call correctly includes
selectedChainIdForSettlementas the third argument, enabling proper cross-chain settlement routing.
595-595: LGTM! UI reflects settlement chain.The USDC display correctly uses
selectedChainIdForSettlementfor the chain logo and test ID, properly indicating where funds will be received.Also applies to: 604-604
src/apps/pulse/components/Sell/tests/Sell.test.tsx (2)
71-85: LGTM!Test props correctly updated to include
portfolioTokens,maxStableCoinBalance,customSellAmounts, andselectedChainIdForSettlement, aligning with the component's new signature.
176-187: Good adjustment for conditional MAX button rendering.The test correctly includes 'MAX' in
customSellAmountsto ensure the MAX button is rendered, reflecting the dynamic button generation based on the preset array.src/apps/pulse/hooks/useRelaySell.ts (4)
45-52: LGTM!The addition of
toChainIdtoSellParamsenables cross-chain settlement. This parameter is properly propagated throughout the sell flow.
132-189: Correct usage oftoChainIdfor destination chain settlement.The USDC address resolution (line 146) and quote request (line 183) correctly use
toChainIdfor the destination chain, enabling users to sell tokens and receive USDC on a different chain.
296-368: Proper chain-aware transaction building.USDC address resolution (line 337) correctly uses
toChainIdfor destination chain targeting, and the fee calculation logic properly computes the 1% platform fee on the settlement chain.
755-788: USDC fee transfer correctly targets settlement chain.The fee transfer transaction (line 768) properly uses
toChainIdas the chainId, ensuring the 1% USDC fee is transferred on the settlement chain where the USDC will be received.src/apps/pulse/components/Settings/SettingsMenu.tsx (4)
63-71: Verify the refresh handler implementation.The refresh handler currently only simulates a 1-second delay without performing any actual refresh logic. Is this intentional as a placeholder, or should it trigger a data refresh (e.g., re-fetching portfolio balances or USDC amounts)?
If a real refresh is needed, consider adding the refresh logic here or documenting that this is intentionally a placeholder for future implementation.
280-314: LGTM! Buy amount input validation is sound.The validation correctly:
- Allows numeric input with optional decimals (line 299)
- Prevents non-numeric characters
- Applies sensible defaults on blur if the field is empty (line 306)
324-362: LGTM! Sell percentage validation properly constrains input.The validation correctly:
- Allows numeric input with optional decimals (line 343)
- Enforces a maximum of 100% (line 345)
- Applies sensible defaults on blur if the field is empty (line 353)
96-132: Proper keyboard and click-outside handling.ESC key handling (lines 97-112) correctly closes the dropdown first before closing the menu, and click-outside handling (lines 115-132) properly closes the dropdown when clicking elsewhere. Both handlers clean up event listeners appropriately.
src/apps/pulse/components/Buy/Buy.tsx (4)
44-86: LGTM! Props correctly refactored for external balance/chain state.The addition of
maxStableCoinBalanceandcustomBuyAmountsprops successfully lifts state management to the parent component, enabling centralized control over balance, chain selection, and buy amount presets.
138-170: LGTM! Balance and chain logic correctly uses external prop.The component now derives
sumOfStableBalancefrommaxStableCoinBalance.balance(line 144) and usesmaxStableCoinBalance.chainIdfor native token lookup (line 154). The dependency array (line 170) correctly includesmaxStableCoinBalance.
192-239: Consider addingmaxStableCoinBalanceto the dependency array.The
useEffectincludessumOfStableBalancein its dependencies (line 233), butsumOfStableBalanceis derived frommaxStableCoinBalance.balance(line 144). To ensure the effect re-runs when the parent updatesmaxStableCoinBalance, consider including it directly in the dependency array.Current approach may work if
sumOfStableBalancealways updates whenmaxStableCoinBalancechanges, but explicit inclusion would make the dependency clearer and more robust:}, [ sumOfStableBalance, usdAmount, setPayingTokens, walletPortfolioData?.result.data, dispensableAssets.length, + maxStableCoinBalance, ]);
600-637: LGTM! Button rendering correctly uses customizable presets.The buttons are now dynamically rendered from
customBuyAmounts(line 602), enabling users to customize their buy amount presets via the SettingsMenu.
| useEffect(() => { | ||
| setSelectedChainIdForSettlement(maxStableCoinBalance.chainId); | ||
| }, [maxStableCoinBalance.chainId]); |
There was a problem hiding this comment.
Preserve user-selected settlement chain
Every time wallet balances refresh (manual refresh or the built-in 15 s timer), maxStableCoinBalance recalculates and this effect force-resets selectedChainIdForSettlement back to the highest-balance chain. That wipes out any manual choice the user made in Settings, so the new “user can change this selection” requirement can’t be satisfied for more than a few seconds. Please gate this auto-sync so it only runs until the user picks a chain themselves.
- useEffect(() => {
- setSelectedChainIdForSettlement(maxStableCoinBalance.chainId);
- }, [maxStableCoinBalance.chainId]);
+ const [hasManualSettlementSelection, setHasManualSettlementSelection] =
+ useState(false);
+
+ const handleManualSettlementSelection = useCallback((chainId: number) => {
+ setSelectedChainIdForSettlement(chainId);
+ setHasManualSettlementSelection(true);
+ }, []);
+
+ useEffect(() => {
+ if (!hasManualSettlementSelection) {
+ setSelectedChainIdForSettlement(maxStableCoinBalance.chainId);
+ }
+ }, [maxStableCoinBalance.chainId, hasManualSettlementSelection]);Then pass handleManualSettlementSelection down to SettingsMenu instead of the raw setSelectedChainIdForSettlement, and flip the manual-selection flag back to false if the user explicitly selects the computed default again.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/pulse/components/App/HomeScreen.tsx around lines 261-263, the effect
currently forces selectedChainIdForSettlement to maxStableCoinBalance.chainId on
every balance refresh, which overwrites a user’s manual choice; change this to
only auto-sync while the user has not made a manual selection by introducing a
local boolean (e.g., hasManuallySelectedSettlement) default false, update the
effect to set selectedChainIdForSettlement from maxStableCoinBalance.chainId
only when hasManuallySelectedSettlement is false, add a
handleManualSettlementSelection function that sets selectedChainIdForSettlement
and sets hasManuallySelectedSettlement to true (and if the user picks the
computed default again, set hasManuallySelectedSettlement back to false), and
pass handleManualSettlementSelection into SettingsMenu instead of directly
passing setSelectedChainIdForSettlement.
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
| /> | ||
| ) : ( | ||
| <> | ||
| <p className="flex text-base font-normal text-white/[.5] w-full text-center mb-6"> |
There was a problem hiding this comment.
Why is it Settings OR displaying the fact that this is a beta version. Teh beta version display should always show no?
There was a problem hiding this comment.
Why is it Settings OR displaying the fact that this is a beta version. Teh beta version display should always show no?
I made the whole home page inside the OR choice not only the para and Aldin asked me to remove that on settings page
There was a problem hiding this comment.
But so where is the beta version showing now?
| setSumOfStableBalance(sum); | ||
| if (maxStableBalance < 2) { | ||
|
|
||
| setSumOfStableBalance(maxStableCoinBalance.balance); |
There was a problem hiding this comment.
from my understanding the maxStableCoinBalance is on a specific chainId right? Should it not be the total of stable balances here?
| onClick={onClose} | ||
| type="button" | ||
| aria-label="Close" | ||
| className="flex items-center justify-center" |
There was a problem hiding this comment.
the component Esc is being used in several components, please make sure this styling is not specific to one component and does not affect other components
There was a problem hiding this comment.
It's doing the same thing and I made it separate just because it shouldn't affect other components and have specifically for settings page
There was a problem hiding this comment.
Was this a design change or something? Just trying to understand why the ESC component changed here as it's a shared component
There was a problem hiding this comment.
No I wanted every button to follow the same design like settings and refresh
| }, [closeSettingsMenu, showChainDropdown]); | ||
|
|
||
| // Handle click outside to close dropdown and tooltip | ||
| useEffect(() => { |
There was a problem hiding this comment.
there is an existing hook for this handle click outside: useClickOutside
| closeSettingsMenu(); | ||
| }; | ||
|
|
||
| // Handle ESC key to close settings |
There was a problem hiding this comment.
There is an existing hook for this handle ESC key: useKeyboardNavigation
| } | ||
|
|
||
| const usdcAddress = getUSDCAddress(fromChainId); | ||
| const usdcAddress = getUSDCAddress(toChainId); |
There was a problem hiding this comment.
Why has this changed from fromChainId to toChainId?
There was a problem hiding this comment.
That's what was the ticket description
| chainId: fromChainId, | ||
| currency: normalizedFromTokenAddress, | ||
| toChainId: fromChainId, | ||
| toChainId, |
| onClick={onClose} | ||
| type="button" | ||
| aria-label="Close" | ||
| className="flex items-center justify-center" |
There was a problem hiding this comment.
Was this a design change or something? Just trying to understand why the ESC component changed here as it's a shared component
| <div | ||
| className="flex flex-col w-full" | ||
| style={{ | ||
| border: '2px solid #1E1D24', | ||
| minHeight: 264, | ||
| backgroundColor: '#1E1D24', | ||
| borderRadius: 16, | ||
| marginTop: 40, | ||
| }} |
There was a problem hiding this comment.
Can we change the inline stiles here
There was a problem hiding this comment.
It was kept as it is that's why will change it to tailwind
| <span style={{ marginLeft: 14, marginRight: 10 }}> | ||
| <img | ||
| src={SearchIcon} | ||
| alt="search-icon" | ||
| width={12} | ||
| height={12} | ||
| /> | ||
| </span> | ||
| <div | ||
| className="flex-1" | ||
| style={{ | ||
| marginLeft: 12, | ||
| backgroundColor: 'black', | ||
| borderRadius: 10, | ||
| width: 40, | ||
| height: 40, | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| display: 'flex', | ||
| padding: '2px 2px 4px 2px', | ||
| color: 'grey', | ||
| textAlign: 'left', | ||
| opacity: 0.5, | ||
| height: 20, | ||
| fontSize: 13, |
There was a problem hiding this comment.
Change this inline styles please to tailwind
There was a problem hiding this comment.
It was kept as it is that's why will change it to tailwind
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
67-74: Convert comment to TODO docblock.The inline comment at line 69 should be formatted as a proper TODO docblock rather than a plain comment.
Apply this diff:
- const handleRefresh = async () => { - setIsRefreshing(true); - // Simulate a refresh action - you can add actual refresh logic here - await new Promise((resolve) => { - setTimeout(resolve, 1000); - }); - setIsRefreshing(false); - }; + /** + * TODO: Implement actual refresh logic + * Currently simulates a refresh action with a 1-second delay + */ + const handleRefresh = async () => { + setIsRefreshing(true); + await new Promise((resolve) => { + setTimeout(resolve, 1000); + }); + setIsRefreshing(false); + };
🧹 Nitpick comments (1)
src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
47-64: Extract default values to constants for maintainability.The default buy amounts and sell percentages are repeated in multiple locations (initialization, onBlur handlers at lines 289 and 336, and validation at lines 80-91). Extracting these to constants would improve maintainability.
Add constants before the component:
+const DEFAULT_BUY_AMOUNTS = ['10', '20', '50', '100']; +const DEFAULT_SELL_PERCENTAGES = ['10', '25', '50', '75']; + export default function SettingsMenu(props: SettingsMenuProps) {Then update state initialization:
- const [buyAmount1, setBuyAmount1] = useState(customBuyAmounts[0] || '10'); - const [buyAmount2, setBuyAmount2] = useState(customBuyAmounts[1] || '20'); - const [buyAmount3, setBuyAmount3] = useState(customBuyAmounts[2] || '50'); - const [buyAmount4, setBuyAmount4] = useState(customBuyAmounts[3] || '100'); + const [buyAmount1, setBuyAmount1] = useState(customBuyAmounts[0] || DEFAULT_BUY_AMOUNTS[0]); + const [buyAmount2, setBuyAmount2] = useState(customBuyAmounts[1] || DEFAULT_BUY_AMOUNTS[1]); + const [buyAmount3, setBuyAmount3] = useState(customBuyAmounts[2] || DEFAULT_BUY_AMOUNTS[2]); + const [buyAmount4, setBuyAmount4] = useState(customBuyAmounts[3] || DEFAULT_BUY_AMOUNTS[3]);Apply similar changes to sell percentages and reference these constants in onBlur handlers and handleConfirm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Settings/SettingsMenu.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
🧬 Code graph analysis (1)
src/apps/pulse/components/Settings/SettingsMenu.tsx (4)
src/utils/blockchain.ts (2)
CompatibleChains(321-323)getLogoForChainId(165-199)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
| <input | ||
| type="text" | ||
| inputMode="numeric" | ||
| value={item.value} | ||
| onChange={(e) => { | ||
| const { value } = e.target; | ||
| // Only allow empty string or valid numbers (including decimals) | ||
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | ||
| item.setter(value); | ||
| } | ||
| }} | ||
| onBlur={(e) => { | ||
| // If empty on blur, set to default | ||
| if (e.target.value === '') { | ||
| item.setter(['10', '20', '50', '100'][index]); | ||
| } | ||
| }} | ||
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | ||
| data-testid={`pulse-settings-buy-amount-${index + 1}`} | ||
| /> |
There was a problem hiding this comment.
Add accessibility labels and consider improving decimal validation.
Two concerns:
-
Accessibility: The input fields lack
aria-labelattributes, which would improve screen reader support. -
Validation edge case: The regex
/^\d*\.?\d*$/allows incomplete decimals like"."or"5.". While the onBlur handler addresses empty strings, it doesn't normalize trailing decimals. For example, if a user types"5."and blurs, the value remains"5."rather than being normalized to"5".
Apply this diff to add aria-labels:
<input
type="text"
inputMode="numeric"
+ aria-label={`Buy amount preset ${index + 1}`}
value={item.value}Optionally, enhance the onBlur handler to normalize trailing decimals:
onBlur={(e) => {
- // If empty on blur, set to default
- if (e.target.value === '') {
+ const val = e.target.value;
+ if (val === '' || val === '.') {
item.setter(['10', '20', '50', '100'][index]);
+ } else if (val.endsWith('.')) {
+ // Remove trailing decimal point
+ item.setter(val.slice(0, -1));
}
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| type="text" | |
| inputMode="numeric" | |
| value={item.value} | |
| onChange={(e) => { | |
| const { value } = e.target; | |
| // Only allow empty string or valid numbers (including decimals) | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| item.setter(value); | |
| } | |
| }} | |
| onBlur={(e) => { | |
| // If empty on blur, set to default | |
| if (e.target.value === '') { | |
| item.setter(['10', '20', '50', '100'][index]); | |
| } | |
| }} | |
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | |
| data-testid={`pulse-settings-buy-amount-${index + 1}`} | |
| /> | |
| <input | |
| type="text" | |
| inputMode="numeric" | |
| aria-label={`Buy amount preset ${index + 1}`} | |
| value={item.value} | |
| onChange={(e) => { | |
| const { value } = e.target; | |
| // Only allow empty string or valid numbers (including decimals) | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| item.setter(value); | |
| } | |
| }} | |
| onBlur={(e) => { | |
| const val = e.target.value; | |
| if (val === '' || val === '.') { | |
| item.setter(['10', '20', '50', '100'][index]); | |
| } else if (val.endsWith('.')) { | |
| // Remove trailing decimal point | |
| item.setter(val.slice(0, -1)); | |
| } | |
| }} | |
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | |
| data-testid={`pulse-settings-buy-amount-${index + 1}`} | |
| /> |
🤖 Prompt for AI Agents
In src/apps/pulse/components/Settings/SettingsMenu.tsx around lines 275 to 294,
add an accessible aria-label to the input (e.g. aria-label={`Buy amount ${index
+ 1}`} or a descriptive label string) to improve screen reader support, and
update the onBlur handler to normalize incomplete decimals: if the value ends
with a trailing dot (e.g. "5.") remove the dot to make "5", and if the value is
just "." replace it with a sensible fallback (either "0" or the component's
default for that index), while keeping the existing behavior that replaces an
empty string with the default.
| <input | ||
| type="text" | ||
| inputMode="numeric" | ||
| value={item.value} | ||
| onChange={(e) => { | ||
| const { value } = e.target; | ||
| // Only allow empty string or valid numbers (including decimals) | ||
| // Also check that the value doesn't exceed 100 | ||
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | ||
| const numValue = parseFloat(value); | ||
| if (value === '' || numValue <= 100) { | ||
| item.setter(value); | ||
| } | ||
| } | ||
| }} | ||
| onBlur={(e) => { | ||
| // If empty on blur, set to default | ||
| if (e.target.value === '') { | ||
| item.setter(['10', '25', '50', '75'][index]); | ||
| } | ||
| }} | ||
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | ||
| data-testid={`pulse-settings-sell-percent-${index + 1}`} | ||
| /> | ||
| <span className="text-white/50 text-sm ml-1">%</span> |
There was a problem hiding this comment.
Add accessibility labels and consider validation feedback.
Two concerns:
-
Accessibility: The input fields lack
aria-labelattributes for screen reader support. -
UX for >100 validation: When a user attempts to type a value greater than 100, the input silently rejects the keystroke with no visual feedback. Consider either:
- Allowing the input and displaying an error message
- Capping the value at 100 on blur
- Providing visual feedback (e.g., red border) when the constraint is violated
Apply this diff to add aria-labels:
<input
type="text"
inputMode="numeric"
+ aria-label={`Sell percentage preset ${index + 1}`}
value={item.value}Optionally, improve validation feedback by capping on blur instead:
onChange={(e) => {
const { value } = e.target;
- // Only allow empty string or valid numbers (including decimals)
- // Also check that the value doesn't exceed 100
- if (value === '' || /^\d*\.?\d*$/.test(value)) {
- const numValue = parseFloat(value);
- if (value === '' || numValue <= 100) {
- item.setter(value);
- }
+ if (value === '' || /^\d*\.?\d*$/.test(value)) {
+ item.setter(value);
}
}}
onBlur={(e) => {
- // If empty on blur, set to default
- if (e.target.value === '') {
+ const val = e.target.value;
+ const numValue = parseFloat(val);
+ if (val === '' || val === '.' || isNaN(numValue)) {
item.setter(['10', '25', '50', '75'][index]);
+ } else if (numValue > 100) {
+ item.setter('100');
+ } else if (val.endsWith('.')) {
+ item.setter(val.slice(0, -1));
}
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| type="text" | |
| inputMode="numeric" | |
| value={item.value} | |
| onChange={(e) => { | |
| const { value } = e.target; | |
| // Only allow empty string or valid numbers (including decimals) | |
| // Also check that the value doesn't exceed 100 | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| const numValue = parseFloat(value); | |
| if (value === '' || numValue <= 100) { | |
| item.setter(value); | |
| } | |
| } | |
| }} | |
| onBlur={(e) => { | |
| // If empty on blur, set to default | |
| if (e.target.value === '') { | |
| item.setter(['10', '25', '50', '75'][index]); | |
| } | |
| }} | |
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | |
| data-testid={`pulse-settings-sell-percent-${index + 1}`} | |
| /> | |
| <span className="text-white/50 text-sm ml-1">%</span> | |
| <input | |
| type="text" | |
| inputMode="numeric" | |
| aria-label={`Sell percentage preset ${index + 1}`} | |
| value={item.value} | |
| onChange={(e) => { | |
| const { value } = e.target; | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| item.setter(value); | |
| } | |
| }} | |
| onBlur={(e) => { | |
| const val = e.target.value; | |
| const numValue = parseFloat(val); | |
| if (val === '' || val === '.' || isNaN(numValue)) { | |
| item.setter(['10', '25', '50', '75'][index]); | |
| } else if (numValue > 100) { | |
| item.setter('100'); | |
| } else if (val.endsWith('.')) { | |
| item.setter(val.slice(0, -1)); | |
| } | |
| }} | |
| className="flex-1 bg-transparent text-white !text-[13px] font-medium outline-none w-full" | |
| data-testid={`pulse-settings-sell-percent-${index + 1}`} | |
| /> | |
| <span className="text-white/50 text-sm ml-1">%</span> |
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (2)
121-130: Consider extracting the hardcoded color value for better maintainability.The arbitrary Tailwind color class
'bg-[#1E1D24]'is hardcoded in the assertions. If this color value changes in the future, the test will need updates. Consider extracting it to a test constant or using a semantic CSS class name if available.Additionally, lines 125-130 use separate assertions to check two classes on the same element, which could be simplified.
Example refactor:
+const ACTIVE_BG_CLASS = 'bg-[#1E1D24]'; +const INACTIVE_BG_CLASS = 'bg-black'; +const INACTIVE_TEXT_CLASS = 'text-grey'; + it('toggles between Buy and Sell components correctly', () => { // ... - // Check button classes for Buy state - expect(screen.getByTestId('pulse-buy-toggle-button')).toHaveClass( - 'bg-[#1E1D24]' - ); - expect(screen.getByTestId('pulse-sell-toggle-button')).toHaveClass( - 'bg-black' - ); - expect(screen.getByTestId('pulse-sell-toggle-button')).toHaveClass( - 'text-grey' - ); + // Check active Buy button styling + expect(screen.getByTestId('pulse-buy-toggle-button')).toHaveClass(ACTIVE_BG_CLASS); + // Check inactive Sell button styling + const sellButton = screen.getByTestId('pulse-sell-toggle-button'); + expect(sellButton).toHaveClass(INACTIVE_BG_CLASS, INACTIVE_TEXT_CLASS);
147-156: Styling assertions are correct; same refactor suggestion applies.The class-based assertions correctly verify the Sell state button styling. The same optional refactor suggestion from lines 121-130 applies here for consistency (extracting the hardcoded color constant and simplifying the multiple assertions).
src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
55-72: Consider using nullish coalescing for edge case correctness.The
||operator treats'0'as falsy, which means ifcustomBuyAmounts[0]orcustomSellAmounts[0]is'0', it would default to the fallback value instead. While'0'is not a practical preset, using??would be more technically correct:- const [buyAmount1, setBuyAmount1] = useState(customBuyAmounts[0] || '10'); + const [buyAmount1, setBuyAmount1] = useState(customBuyAmounts[0] ?? '10');Apply similar changes to all other state initializations (lines 56-72).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/App/tests/HomeScreen.test.tsx(3 hunks)src/apps/pulse/components/Settings/SettingsMenu.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Settings/SettingsMenu.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Settings/SettingsMenu.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Settings/SettingsMenu.tsx (4)
src/utils/blockchain.ts (2)
CompatibleChains(321-323)getLogoForChainId(165-199)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (3)
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
106-106: LGTM! Label correctly updated to Settings.The test assertion properly reflects the UI change from "Save" to "Settings" as described in the PR objectives.
src/apps/pulse/components/Settings/SettingsMenu.tsx (2)
85-105: Validation pattern is sound, with same edge case as state initialization.The validation logic correctly provides fallbacks for empty strings. Note that this function uses the same
||operator pattern as the state initialization (lines 55-72), so the'0'edge case applies here as well.Also, values from the input fields (e.g.,
buyAmount1) are passed directly, so any incomplete decimals like"5."that make it through the input validation would be propagated to the parent state. This relates to the decimal normalization concerns in past review comments.
107-370: Well-structured component with correct hook usage.The implementation demonstrates several strengths:
- Proper hook integration:
useKeyboardNavigationanduseClickOutsideare correctly used, addressing previous review feedback.- Clear component structure: Settings sections are well-organized with consistent styling and layout.
- State management: The chain dropdown and input field states are properly managed with appropriate close/reset behaviors.
- ESC key handling: Correctly prioritizes closing the dropdown first (line 110-111), then the menu itself (line 113), providing good UX.
- Test coverage: Comprehensive
data-testidattributes for testing.Note: Previous review comments on lines 283-302 and 326-350 regarding accessibility (missing
aria-labelattributes) and decimal normalization remain valid points for improvement.
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
226-228: User-selected settlement chain is overwritten on every balance refresh.This issue was already flagged in a previous review: every time wallet balances refresh (manual refresh or the 15s timer), this effect force-resets
selectedChainIdForSettlementback tomaxStableCoinBalance.chainId, wiping out any manual choice the user made in Settings. This directly violates the PR requirement that "the user can override this selection."Please implement the gating logic suggested in the previous review to preserve user selections:
+ const [hasManualSettlementSelection, setHasManualSettlementSelection] = + useState(false); + + const handleManualSettlementSelection = useCallback((chainId: number) => { + setSelectedChainIdForSettlement(chainId); + setHasManualSettlementSelection(true); + }, []); + useEffect(() => { - setSelectedChainIdForSettlement(maxStableCoinBalance.chainId); - }, [maxStableCoinBalance.chainId]); + if (!hasManualSettlementSelection) { + setSelectedChainIdForSettlement(maxStableCoinBalance.chainId); + } + }, [maxStableCoinBalance.chainId, hasManualSettlementSelection]);Then pass
handleManualSettlementSelectiontoSettingsMenu(line 924) instead ofsetSelectedChainIdForSettlement, and resethasManualSettlementSelectiontofalseif the user explicitly picks the computed default again.
🧹 Nitpick comments (2)
src/apps/pulse/utils/utils.tsx (1)
213-253: Refactor to eliminate duplicate filtering and improve performance.The function filters assets twice with identical logic (lines 229-236 and 239-244), then iterates over the results with nested loops. This is inefficient for large portfolios.
Consider this refactor:
export const getStableCurrencyBalanceOnEachChain = ( walletPortfolioData: WalletPortfolioMobulaResponse ): { [chainId: number]: number } => { - // get the list of chainIds from STABLE_CURRENCIES const chainIds = Array.from( new Set(STABLE_CURRENCIES.map((currency) => currency.chainId)) ); - // create a map to hold the balance for each chainId const balanceMap: { [chainId: number]: number } = {}; chainIds.forEach((chainId) => { balanceMap[chainId] = 0; }); - // calculate the balance for each chainId + + // Build a lookup map for faster stable currency checks + const stableLookup = new Map<string, number>(); + STABLE_CURRENCIES.forEach((stable) => { + const key = `${stable.chainId}:${stable.address.toLowerCase()}`; + stableLookup.set(key, stable.chainId); + }); + walletPortfolioData?.result.data.assets - ?.filter((asset) => - asset.contracts_balances.some((contract) => - STABLE_CURRENCIES.some( - (stable) => - stable.address.toLowerCase() === contract.address.toLowerCase() && - stable.chainId === Number(contract.chainId.split(':').at(-1)) - ) - ) - ) .forEach((asset) => { - const stableContracts = asset.contracts_balances.filter((contract) => - STABLE_CURRENCIES.some( - (stable) => - stable.address.toLowerCase() === contract.address.toLowerCase() && - stable.chainId === Number(contract.chainId.split(':').at(-1)) - ) - ); - stableContracts.forEach((contract) => { + asset.contracts_balances.forEach((contract) => { const chainId = Number(contract.chainId.split(':').at(-1)); - balanceMap[chainId] += asset.price * contract.balance; + const key = `${chainId}:${contract.address.toLowerCase()}`; + if (stableLookup.has(key)) { + const price = asset.price ?? 0; + const balance = contract.balance ?? 0; + balanceMap[chainId] += price * balance; + } }); }); return balanceMap; };This eliminates the duplicate filter and reduces the O(n×m×k) complexity to O(n×m) by using a Map lookup.
src/apps/pulse/components/App/HomeScreen.tsx (1)
107-110: Clarify the default balance of 2.The comment "Default to 2 to allow trading initially" is helpful, but the magic number
2might confuse future maintainers. Is this a minimum threshold documented elsewhere?Consider extracting this as a named constant:
+const DEFAULT_MIN_STABLE_BALANCE_FOR_TRADING = 2; + export default function HomeScreen(props: HomeScreenProps) { // ... const [maxStableCoinBalance, setMaxStableCoinBalance] = useState<{ chainId: number; balance: number; - }>({ chainId: 1, balance: 2 }); // Default to 2 to allow trading initially + }>({ chainId: 1, balance: DEFAULT_MIN_STABLE_BALANCE_FOR_TRADING });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/App/HomeScreen.tsx(8 hunks)src/apps/pulse/utils/utils.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/utils/utils.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
🧬 Code graph analysis (2)
src/apps/pulse/utils/utils.tsx (2)
src/types/api.ts (1)
WalletPortfolioMobulaResponse(750-752)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/utils/utils.tsx (1)
getStableCurrencyBalanceOnEachChain(214-253)src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
SettingsMenu(32-370)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)src/apps/pulse/components/Misc/Settings.tsx (1)
Settings(7-21)src/apps/pulse/components/Buy/Buy.tsx (1)
Buy(69-663)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
916-1032: LGTM: Settings menu integration is clean.The conditional rendering of
SettingsMenuvs. the main Buy/Sell UI is clear, and the props passed to Buy, Sell, PreviewSell, and SettingsMenu correctly wire up the new settlement chain and custom amounts features. The UI restructure aligns with the PR objectives.
| const maxStableBalance = Math.max(...Object.values(stableBalance)); | ||
| const chainIdOfMaxStableBalance = Number( | ||
| Object.keys(stableBalance).find( | ||
| (key) => stableBalance[Number(key)] === maxStableBalance | ||
| ) || '1' | ||
| ); |
There was a problem hiding this comment.
Handle edge case when stable balance map is empty.
If getStableCurrencyBalanceOnEachChain returns an empty object (no stable currencies found), Math.max(...Object.values(stableBalance)) returns -Infinity, and .find(...) returns undefined. The fallback || '1' prevents a crash, but you'll set balance to -Infinity.
Apply this diff to default to a safe state:
const stableBalance =
getStableCurrencyBalanceOnEachChain(walletPortfolioData);
+ if (Object.keys(stableBalance).length === 0) {
+ setMaxStableCoinBalance({ chainId: 1, balance: 0 });
+ return;
+ }
const maxStableBalance = Math.max(...Object.values(stableBalance));
const chainIdOfMaxStableBalance = Number(
Object.keys(stableBalance).find(
(key) => stableBalance[Number(key)] === maxStableBalance
) || '1'
);Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3833-USDC-directional-changes-for-Sell-sides)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
220-222: Preserve user-selected settlement chain.This effect overwrites user-selected settlement chains on every balance refresh (15s timer). The issue and solution are already documented in previous review comments.
🧹 Nitpick comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
201-217: Consider removing portfolioTokens dependency.The effect only uses
walletPortfolioDatato compute stable balances (line 206), but the early return also checksportfolioTokenswhich is derived separately (lines 790-798). This dependency might cause unnecessary delays ifportfolioTokenshasn't populated yet whenwalletPortfolioDataarrives.Consider this refactor:
useEffect(() => { - if (!walletPortfolioData || !portfolioTokens || portfolioTokens.length === 0) { + if (!walletPortfolioData) { return; } const stableBalance = getStableCurrencyBalanceOnEachChain(walletPortfolioData); + if (Object.keys(stableBalance).length === 0) { + setMaxStableCoinBalance({ chainId: 1, balance: 0 }); + return; + } const maxStableBalance = Math.max(...Object.values(stableBalance)); const chainIdOfMaxStableBalance = Number( Object.keys(stableBalance).find( (key) => stableBalance[Number(key)] === maxStableBalance ) || '1' ); setMaxStableCoinBalance({ chainId: chainIdOfMaxStableBalance, balance: maxStableBalance, }); - }, [portfolioTokens, walletPortfolioData]); + }, [walletPortfolioData]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/App/HomeScreen.tsx(8 hunks)src/apps/pulse/utils/utils.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/utils/utils.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/utils/utils.tsx (1)
getStableCurrencyBalanceOnEachChain(214-255)src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
SettingsMenu(32-370)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)src/apps/pulse/components/Misc/Settings.tsx (1)
Settings(7-21)src/apps/pulse/components/Buy/Buy.tsx (1)
Buy(69-663)
⏰ 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 (4)
src/apps/pulse/components/App/HomeScreen.tsx (4)
35-35: LGTM! Clean integration of new settings and balance tracking.The imports and state declarations correctly introduce SettingsMenu, balance computation utilities, and configurable buy/sell presets. The default stable balance of 2 allows trading initially while waiting for actual portfolio data.
Also applies to: 43-43, 107-135
193-199: LGTM! Wallet portfolio query configured correctly.The hook correctly skips queries when no account is connected and disables refetch-on-focus to prevent unnecessary polling.
268-268: LGTM! Settlement chain correctly propagated through sell flow.The
selectedChainIdForSettlementis properly used in sell offer fetching (line 268), included in effect dependencies (line 298), and passed to PreviewSell (line 864).Also applies to: 298-298, 864-864
909-1027: LGTM! Clean UI restructure with settings integration.The conditional rendering between SettingsMenu and main trading UI is well-structured. Props are correctly threaded to child components: Buy receives
maxStableCoinBalanceandcustomBuyAmounts, Sell receivescustomSellAmountsandselectedChainIdForSettlement. The refresh button is appropriately disabled when no token is selected.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/apps/pulse/components/App/HomeScreen.tsx (2)
211-216: Handle edge case when stable balance map is empty.If
getStableCurrencyBalanceOnEachChainreturns an empty object (no stable currencies found),Math.max(...Object.values(stableBalance))returns-Infinity, and the fallback|| '1'prevents a crash but sets balance to-Infinity. This was flagged in a previous review and remains unaddressed.Apply this diff to default to a safe state:
const stableBalance = getStableCurrencyBalanceOnEachChain(walletPortfolioData); + if (Object.keys(stableBalance).length === 0) { + setMaxStableCoinBalance({ chainId: 1, balance: 0 }); + return; + } const maxStableBalance = Math.max(...Object.values(stableBalance)); const chainIdOfMaxStableBalance = Number( Object.keys(stableBalance).find( (key) => stableBalance[Number(key)] === maxStableBalance ) || '1' );
224-226: Preserve user-selected settlement chain.Every time wallet balances refresh (manual refresh or the built-in 15s timer),
maxStableCoinBalancerecalculates and this effect force-resetsselectedChainIdForSettlementback to the highest-balance chain. That wipes out any manual choice the user made in Settings, so the new "user can change this selection" requirement cannot be satisfied for more than a few seconds.Please gate this auto-sync so it only runs until the user picks a chain themselves:
- useEffect(() => { - setSelectedChainIdForSettlement(maxStableCoinBalance.chainId); - }, [maxStableCoinBalance.chainId]); + const [hasManualSettlementSelection, setHasManualSettlementSelection] = + useState(false); + + const handleManualSettlementSelection = useCallback((chainId: number) => { + setSelectedChainIdForSettlement(chainId); + setHasManualSettlementSelection(true); + }, []); + + useEffect(() => { + if (!hasManualSettlementSelection) { + setSelectedChainIdForSettlement(maxStableCoinBalance.chainId); + } + }, [maxStableCoinBalance.chainId, hasManualSettlementSelection]);Then pass
handleManualSettlementSelectiondown toSettingsMenu(line 922) instead of the rawsetSelectedChainIdForSettlement, and reset the manual-selection flag back tofalseif the user explicitly selects the computed default again.
🧹 Nitpick comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
107-110: Consider clarifying the default balance value.The default balance of
2with the comment "Default to 2 to allow trading initially" may be misleading. This default will be immediately replaced by the computed value from the effect at lines 201-221 oncewalletPortfolioDataloads. If the intent is to allow trading before portfolio data loads, consider documenting this more clearly or using a more meaningful initial value (e.g.,0with a loading state).
📜 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 (1)
src/apps/pulse/components/App/HomeScreen.tsx(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/HomeScreen.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/utils/utils.tsx (1)
getStableCurrencyBalanceOnEachChain(214-255)src/apps/pulse/components/Settings/SettingsMenu.tsx (1)
SettingsMenu(32-370)src/apps/pulse/components/Misc/Refresh.tsx (1)
Refresh(10-35)src/apps/pulse/components/Misc/Settings.tsx (1)
Settings(7-21)src/apps/pulse/components/Buy/Buy.tsx (1)
Buy(69-663)
⏰ 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 (3)
src/apps/pulse/components/App/HomeScreen.tsx (3)
193-199: LGTM!The wallet portfolio query setup is correct, with appropriate skip conditions and refetch configuration.
272-272: LGTM!The
selectedChainIdForSettlementis correctly passed to both the sell offer retrieval and PreviewSell component, properly threading the settlement chain preference through the sell flow.Also applies to: 868-868
913-1031: LGTM!The UI restructure integrating SettingsMenu is well-implemented. The conditional rendering between Settings and the main trading UI is clear, props are correctly threaded through to Buy/Sell components, and the spread syntax adding 'MAX' to custom amounts works correctly.
Note: The SettingsMenu integration at line 922 directly passes
setSelectedChainIdForSettlement, which connects to the critical issue flagged above regarding auto-reset of user's manual selection.
vignesha22
deleted the
feature/PRO-3833-USDC-directional-changes-for-Sell-sides
branch
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Behavior
Style
Tests