Conversation
WalkthroughAdds a gas‑tank onboarding and top‑up flow: new onboarding UI and preview, top‑up orchestration and gas‑estimation hooks, gas‑tank/USDC balance hooks, AppWrapper→HomeScreen/Search prop wiring, PortfolioTokenList stable‑coin toggle, batch exclusions for "pulse-topup", tests, and safer paymaster URL handling. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant AppWrapper
participant HomeScreen
participant OnboardingWelcome
participant TopUpScreen
participant PreviewTopUp
participant IntentSdk
participant PaymasterAPI
participant Poller
User->>AppWrapper: open app
AppWrapper->>HomeScreen: render (pass onboarding props)
HomeScreen->>PaymasterAPI: fetch gas tank balance (useGasTankBalance)
alt onboarding needed (low gas && not completed)
HomeScreen->>OnboardingWelcome: render welcome
User->>OnboardingWelcome: click "Top up"
OnboardingWelcome->>HomeScreen: setOnboardingScreen("topup")
HomeScreen->>TopUpScreen: render top‑up UI
TopUpScreen->>IntentSdk: request quote / sell offer (if non‑USDC)
User->>TopUpScreen: confirm -> open PreviewTopUp
PreviewTopUp->>IntentSdk: check modules & build/send batch (useTopUp / getEnablePulseTradingTransactions)
PreviewTopUp->>PaymasterAPI: fetch deposit transactions
IntentSdk->>PreviewTopUp: return userOpHash
Poller->>PreviewTopUp: poll userOp status -> update UI
PreviewTopUp->>HomeScreen: mark onboarding complete / refetch balances
HomeScreen->>User: render main interface
else
HomeScreen->>User: render main interface
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 6
🧹 Nitpick comments (17)
src/providers/GlobalTransactionsBatchProvider.tsx (1)
86-86: Consider adding an explanatory comment for the pulse-topup exclusion.For consistency and maintainability, consider adding a comment explaining why pulse-topup batches are excluded, similar to the existing comments for pulse-sell and pulse-buy on lines 79-81.
// Filter out pulse-sell, pulse-buy and paymaster-batch from the count // - pulse-sell and pulse-buy batches are handled directly in the Pulse app + // - pulse-topup batches are handled directly in the Pulse onboarding flow // - paymaster-batch are internal and should not be displayed in batch counts const filteredBatches = Object.keys(batches).filter(src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (1)
82-90: Add pulse-topup filtering to the useMemo for consistency.The
groupedBatchesByChainIduseMemo filters out pulse-sell and pulse-buy batches, but pulse-topup batches are only filtered during rendering (lines 747-748). For consistency and to ensure accurate batch grouping, consider adding the pulse-topup filter here as well.Object.entries(batches).forEach(([batchName, transactions]) => { if (!transactions.length) return; // Skip pulse-sell and pulse-buy batches - they are handled directly in the Pulse app if (batchName.includes('pulse-sell')) return; if (batchName.includes('pulse-buy')) return; + // Skip pulse-topup batches - they are handled directly in the Pulse onboarding flow + if (batchName.includes('pulse-topup')) return; // Skip paymaster-batch - these are internal and should not be displayed in batches view if (batchName === 'paymaster-batch') return;src/apps/pulse/hooks/useGasTankBalance.ts (3)
3-6: Consider exportingChainBalanceinterface for external use.The
ChainBalanceinterface may be useful for consumers of this hook who want to type their usage ofchainBalances. Consider exporting it if needed elsewhere.
32-79: Add AbortController to prevent state updates after unmount.The fetch operation lacks cancellation handling. If the component unmounts while a request is in-flight, calling
setTotalBalance,setChainBalances, etc. will attempt to update unmounted component state.const fetchGasTankBalance = async () => { + const abortController = new AbortController(); + if (!walletAddress || !paymasterUrl) { setTotalBalance(0); setChainBalances([]); - return; + return abortController; } setIsLoading(true); setError(null); try { const response = await fetch( `${paymasterUrl}/getGasTankBalance?sender=${walletAddress}`, { method: 'GET', headers: { 'Content-Type': 'application/json', }, + signal: abortController.signal, } ); // ... rest of the logic } catch (err) { + if (err instanceof Error && err.name === 'AbortError') { + return abortController; + } // ... existing error handling } finally { setIsLoading(false); } + return abortController; };Then in the
useEffect, return a cleanup function that callsabortController.abort().
81-84: Consider usinguseCallbackto avoid eslint-disable.Wrapping
fetchGasTankBalanceinuseCallbackwith[walletAddress, paymasterUrl]dependencies would eliminate the need for the eslint-disable comment and make the dependency relationship explicit.src/apps/pulse/components/Onboarding/tests/PreviewTopUp.integration.test.tsx (2)
169-173: Duplicate test case.This test (
'renders gas fee display'at line 170) duplicates the test at lines 118-122. Consider removing this duplicate or making it test something distinct about loading states.describe('loading states', () => { - it('renders gas fee display', () => { - render(<PreviewTopUp {...createMockProps()} />); - expect(screen.getByText('Gas fee:')).toBeInTheDocument(); - }); - it('renders confirm button in enabled state by default', () => {
184-189: Consider adding more error state tests.The error handling section only verifies the component renders without errors. Consider adding tests for when
useTopUpreturns an error or whengasEstimationErroris set in the mock.src/apps/pulse/hooks/useGasEstimationTopUp.ts (1)
161-185: Potential stale closure when invokingestimateGasFeesRef.current.The
useEffectdependency array includesallocateAmountandsellOffer, but the effect body only checksestimateGasFeesRef.current. Since the ref is updated after the callback is created (Line 162), and the effect runs when dependencies change, this pattern is correct. However, consider adding a brief comment explaining the ref pattern for maintainability.Additionally,
userPortfoliois missing from the dependency array but is part of theestimateGasFeescallback dependencies. IfuserPortfoliochanges, the effect won't re-run, potentially causing stale estimations.}, [ selectedToken, kit, amount, allocateAmount, sellOffer, isPaused, isInitialized, + userPortfolio, ]);src/apps/pulse/hooks/useTopUp.ts (3)
95-114: Network request lacks timeout and retry logic.The
fetchcall to the paymaster API has no timeout configuration. If the paymaster service is slow or unresponsive, this could leave the user waiting indefinitely.Consider adding an
AbortControllerwith a timeout:+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout + const response = await fetch( `${paymasterUrl}/getTransactionForDeposit?chainId=${chainId}&amount=${allocateAmount}`, { method: 'GET', headers: { 'Content-Type': 'application/json', }, + signal: controller.signal, } ); + + clearTimeout(timeoutId);
119-141: Paymaster API response validation could be more robust.The response handling checks for array vs object but doesn't validate the structure of individual transaction objects (e.g., verifying
tois a valid address). While the subsequent batch execution would likely fail on malformed data, explicit validation could provide clearer error messages.
304-309:buildTopUpTransactionsForEstimationis a redundant wrapper.This function simply delegates to
buildTopUpTransactionswithout any additional logic. Consider exportingbuildTopUpTransactionsdirectly or adding a comment explaining why the indirection exists (e.g., future extensibility).src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
639-691: Step rendering uses array index as key.Line 640 uses
indexas the React key for step items. Since the steps array is dynamically generated based on conditions, the order and content can change, making index-based keys potentially problematic for React's reconciliation.Consider using a stable identifier:
- {steps.map((step, index) => ( - <div key={index}> + {steps.map((step) => ( + <div key={step.title}>src/apps/pulse/components/App/HomeScreen.tsx (1)
266-312: USDC preselection effect has redundant condition check.The effect filters
portfolioTokensto find USDC tokens and selects the one with the highest balance. The logic is correct, butwalletPortfolioData?.result?.data?.assetscheck at line 272 seems unnecessary sinceportfolioTokensis already derived fromwalletPortfolioDataelsewhere.Consider simplifying:
if ( onboardingScreen === 'topup' && totalUsdcBalance > 0 && - !topupToken && - walletPortfolioData?.result?.data?.assets + !topupToken ) {src/apps/pulse/components/Onboarding/TopUpScreen.tsx (4)
59-66:isSelectedTokenUSDCfunction is redefined on every render.This function is called multiple times throughout the component but is recreated on each render. Consider memoizing it with
useCallbackor moving the logic inline where needed.- const isSelectedTokenUSDC = (): boolean => { + const isSelectedTokenUSDC = useCallback((): boolean => { if (!selectedToken) return false; return STABLE_CURRENCIES.some( (stable) => stable.chainId === selectedToken.chainId && stable.address.toLowerCase() === selectedToken.address.toLowerCase() ); - }; + }, [selectedToken]);
137-199: Quote fetching effect lacks debouncing.The effect fetches a new quote every time
amountchanges. Rapid typing could trigger many API calls. Consider adding debouncing to reduce unnecessary network requests.Example with a simple debounce:
useEffect(() => { const timeoutId = setTimeout(() => { fetchSellOffer(); }, 500); // 500ms debounce return () => clearTimeout(timeoutId); }, [amount, selectedToken, ...]);
214-234: Unnecessary eslint-disable comment.Line 214 has
@typescript-eslint/no-use-before-definedisable, buthandleUsdAmountChangeis defined before it's used in the JSX. This comment appears to be a leftover.Remove the unnecessary eslint-disable:
- // eslint-disable-next-line @typescript-eslint/no-use-before-define const handleUsdAmountChange = (e: React.ChangeEvent<HTMLInputElement>) => {
640-675: Button disabled logic is duplicated betweendisabledandstyle.The same condition is used for both the
disabledattribute and the background color style. Consider extracting this to a variable for DRY compliance.+ const isButtonDisabled = + parseFloat(amount) <= 0 || + isPortfolioLoading || + portfolioTokens.length === 0 || + isLoadingQuote || + (!isSelectedTokenUSDC() && !sellOffer && parseFloat(amount) > 0) || + !!relayError || + !!error; + <button onClick={handleTopUp} type="button" className="..." style={{ - backgroundColor: - parseFloat(amount) <= 0 || - ... - ? '#29292F' - : '#8A77FF', + backgroundColor: isButtonDisabled ? '#29292F' : '#8A77FF', }} - disabled={...} + disabled={isButtonDisabled} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/apps/pulse/assets/circle-arrow-down.svgis excluded by!**/*.svgsrc/apps/pulse/assets/gas-tank-icon.svgis excluded by!**/*.svgsrc/apps/pulse/assets/install-7702-icon.svgis excluded by!**/*.svgsrc/apps/pulse/assets/onboarding-gas-tank-icon.svgis excluded by!**/*.svgsrc/apps/pulse/assets/onboarding-install-modules-icon.svgis excluded by!**/*.svgsrc/apps/pulse/assets/onboarding-swap-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
src/apps/pulse/components/App/AppWrapper.tsx(3 hunks)src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)src/apps/pulse/components/App/tests/AppWrapper.test.tsx(3 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(4 hunks)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx(1 hunks)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/PreviewTopUp.integration.test.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(3 hunks)src/apps/pulse/components/Search/Search.tsx(3 hunks)src/apps/pulse/hooks/useGasEstimationTopUp.ts(1 hunks)src/apps/pulse/hooks/useGasTankBalance.ts(1 hunks)src/apps/pulse/hooks/useIntentSdk.ts(3 hunks)src/apps/pulse/hooks/useTopUp.ts(1 hunks)src/apps/pulse/hooks/useTotalUsdcBalance.ts(1 hunks)src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx(1 hunks)src/providers/GlobalTransactionsBatchProvider.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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-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/Search/Search.tsxsrc/apps/pulse/components/App/AppWrapper.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 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/hooks/useIntentSdk.tssrc/apps/pulse/hooks/useTopUp.tssrc/apps/pulse/hooks/useTotalUsdcBalance.tssrc/apps/pulse/hooks/useGasEstimationTopUp.tssrc/apps/pulse/components/Onboarding/TopUpScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.tssrc/apps/pulse/components/App/AppWrapper.tsxsrc/apps/pulse/components/App/tests/HomeScreen.test.tsxsrc/apps/pulse/hooks/useGasTankBalance.tssrc/apps/pulse/components/App/tests/AppWrapper.test.tsxsrc/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/tests/HomeScreen.test.tsxsrc/apps/pulse/hooks/useTopUp.tssrc/apps/pulse/components/App/tests/AppWrapper.test.tsxsrc/apps/pulse/components/App/HomeScreen.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/Onboarding/TopUpScreen.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
🧬 Code graph analysis (7)
src/apps/pulse/components/Onboarding/tests/PreviewTopUp.integration.test.tsx (1)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
PreviewTopUp(73-784)
src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx (1)
src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(13-104)
src/apps/pulse/components/App/AppWrapper.tsx (1)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)
src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx (1)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(33-679)
src/apps/pulse/hooks/useTopUp.ts (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
Token(19-29)src/apps/pulse/hooks/useRelaySell.ts (1)
useRelaySell(54-1610)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/utils/eip7702Authorization.ts (1)
getEIP7702AuthorizationIfNeeded(16-95)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (6)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
PortfolioToken(31-34)src/apps/pulse/hooks/useRelaySell.ts (1)
useRelaySell(54-1610)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
PreviewTopUp(73-784)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)
src/apps/pulse/components/App/HomeScreen.tsx (7)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-93)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(13-104)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(33-679)
⏰ 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: Cloudflare Pages: pillarx-debug
🔇 Additional comments (20)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
30-30: LGTM! Clean implementation of stable coin filtering.The new
includeStableCoinsprop with a default value offalsemaintains backward compatibility while enabling the onboarding flow to include stable currencies when needed. The filtering logic is clear and correct.Also applies to: 40-40, 53-77
src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
1-35: LGTM! Well-structured custom hook.The implementation correctly uses
useMemoto optimize the balance calculation, properly handles undefined input, and has the correct dependency array. The explicit return type interface enhances type safety.src/apps/pulse/components/Search/Search.tsx (1)
51-51: LGTM! Clean prop threading for onboarding flow.The
isSearchingFromTopupprop is cleanly threaded through the component and semantically mapped toincludeStableCoinswhen passed to PortfolioTokenList. The implementation integrates well with the existing search flow.Also applies to: 77-77, 452-452
src/apps/pulse/components/App/tests/HomeScreen.test.tsx (1)
9-9: LGTM! Test setup properly updated for onboarding flow.The test correctly mocks
useGasTankBalanceand adds the new onboarding-related props tomockProps. The mock configuration ensures existing tests continue to pass by returning a non-zero gas tank balance, which displays the main interface instead of the onboarding flow.Also applies to: 33-35, 47-51, 90-93
src/apps/pulse/components/App/AppWrapper.tsx (1)
17-21: LGTM! Clean state management for onboarding flow.The new state variables are well-typed and semantically named. The
onboardingScreenuses a clear discriminated union type ('welcome' | 'topup' | null), and the props are correctly threaded to the appropriate child components.Also applies to: 80-80, 92-96
src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx (1)
1-187: Well-structured test suite with comprehensive coverage.The test suite covers rendering, balance formatting, button interactions, loading states, and accessibility. Good use of
userEvent.setup()for async interactions and properbeforeEachcleanup.src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
79-100: LGTM - Button implementation is well-structured.The button correctly handles loading state with spinner, disabled state, and has proper
type="button"anddata-testidattributes for testing.src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx (2)
68-162: Good test coverage for TopUpScreen.The test suite covers component rendering, user interactions, loading states, token selection, and accessibility. The structure mirrors the other onboarding test files for consistency.
12-18: I'll verify whether the mocks for these hooks are actually incomplete and cause test failures by examining the TopUpScreen component and the actual hook implementations.
<call_capability>
<capability_name>shell
#!/bin/bashFirst, find TopUpScreen.tsx to see if it uses these hooks
find . -name "TopUpScreen.tsx" -type f | head -5
</call_capability>src/apps/pulse/hooks/useIntentSdk.ts (2)
152-181: Good addition ofmoduleCheckErrorto prevent retry loops.The
moduleCheckErrorstate prevents repeated failed module checks, avoiding potential infinite loops or excessive API calls after an error. The integration with theuseEffectdependencies is correct.
179-181: VerifytransactionDebugLogstability.The
eslint-disablecomment is used, buttransactionDebugLogfromuseTransactionDebugLoggeris used inside the effect. Ensure this function reference is stable (memoized) to avoid unexpected re-runs.src/apps/pulse/hooks/useGasEstimationTopUp.ts (1)
1-35: LGTM! Clean hook initialization with proper state management.The hook setup correctly uses refs for preventing concurrent estimations and properly initializes state for gas estimation tracking.
src/apps/pulse/hooks/useTopUp.ts (2)
70-77: Floating-point division for token amount calculation.The token amount calculation uses floating-point arithmetic which can lead to precision issues with very small or very large token values. Based on learnings, the team has accepted this trade-off for the Pulse app.
261-277: Success path returns beforesetIsLoading(false)is called in finally block.Lines 272-273 call
setIsLoading(false)and return before reaching thefinallyblock. While this works correctly, the pattern is inconsistent—the success path manually resets loading state while the error path relies on line 294. This is fine but worth noting for consistency.src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
454-458: Polling interval not cleared on component unmount during active polling.The polling
setIntervalat line 454 returns a cleanup function, but ifisPollingActivebecomes false before unmount (e.g., viacloseTransactionStatus), the interval is already cleared. This is correct. However, verify that all paths that stop polling also clear any pending timeouts fromupdateStatusWithDelay.src/apps/pulse/components/App/HomeScreen.tsx (3)
220-223: UsinguseReffor localStorage-backed state.The
hasCompletedOnboardingRefpattern correctly persists onboarding completion across rerenders without causing effect re-runs. This is a valid pattern for derived state from localStorage.
240-264: Race condition between loading states and onboarding decision.The effect depends on
isGasTankLoading,walletPortfolioData, andportfolioTokens.length. However, there's a potential timing issue: ifgasTankBalanceis 0 initially but updates to a positive value after the effect runs, the onboarding screen might briefly flash.The condition at line 253 (
gasTankBalance > 0 || hasCompletedOnboardingRef.current) handles this by dismissing onboarding when balance becomes positive, which is correct.
1069-1092: Loading state shows onboarding welcome during initial load.Lines 1077-1091 show
OnboardingWelcomewith loading state whengasTankBalance === 0and onboarding hasn't been completed, even before portfolio data is fully loaded. This prevents flashing the main UI, which is the intended behavior per the comment.src/apps/pulse/components/Onboarding/TopUpScreen.tsx (2)
107-135: MissingisSelectedTokenUSDCin dependency array causes stale closures.The
useEffectat line 107 usesisSelectedTokenUSDC()but doesn't include it in the dependency array. SinceisSelectedTokenUSDCdepends onselectedToken, andselectedTokenis in the deps, this works by coincidence. However, ifisSelectedTokenUSDCwere memoized, this could become a bug.The eslint-disable comment at line 134 suppresses the warning, but consider including the function explicitly if memoized.
295-342:handleTopUpvalidation logic is comprehensive.The validation covers all edge cases: empty amount, no token, minimum allocation, non-USDC without quote, and balance check. This is well-structured.
src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx
Show resolved
Hide resolved
| <span className="text-sm font-normal text-white"> | ||
| Universal Gas Tank: $ | ||
| {gasTankBalance.toLocaleString('en-US', { | ||
| minimumFractionDigits: 2, |
There was a problem hiding this comment.
why min and max are the same?
There was a problem hiding this comment.
if its 2 then it will display 2.00 like in design thats why
There was a problem hiding this comment.
why not doing a toFixed(2) ?
| console.error(errorMessage); | ||
| // Clean up the batch on error | ||
| const { selectedToken } = params; | ||
| const batchName = `pulse-topup-batch-${selectedToken.chainId}`; |
There was a problem hiding this comment.
I noticed that the pulse-topup-batch is used in useGasEstimation too
Deploying pillarx-debug with
|
| Latest commit: |
f5b9058
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c6fccf2a.pillarx-debug.pages.dev |
| Branch Preview URL: | https://pro-3733-onboardingflow.pillarx-debug.pages.dev |
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
Deploying x with
|
| Latest commit: |
f5b9058
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9d26d085.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3733-onboardingflow.x-e62.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
145-160: Avoid dummySelectedTokenwhenselectedTokenis nullRight now the hook is called with a fabricated
SelectedTokenwhenselectedTokenis null and relies onisPausedto prevent real work. It works, but it’s brittle and can hide bugs ifuseGasEstimationTopUpever changes.Consider updating
useGasEstimationTopUpto acceptSelectedToken | null(and early‑return when null), or only calling the hook whenselectedTokenis defined, instead of passing a dummy object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx(1 hunks)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx(1 hunks)src/apps/pulse/hooks/useGasEstimationTopUp.ts(1 hunks)src/apps/pulse/hooks/useIntentSdk.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/apps/pulse/hooks/useGasEstimationTopUp.ts
- src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx
- src/apps/pulse/components/Onboarding/tests/TopUpScreen.integration.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/hooks/useIntentSdk.tssrc/apps/pulse/components/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/hooks/useIntentSdk.ts
📚 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/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.tsx
🧬 Code graph analysis (2)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (9)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
PortfolioToken(31-34)src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(24-230)src/apps/pulse/hooks/useTopUp.ts (1)
useTopUp(30-319)src/apps/pulse/hooks/useGasEstimationTopUp.ts (1)
useGasEstimationTopUp(16-196)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
PortfolioToken(31-34)src/apps/pulse/hooks/useRelaySell.ts (1)
useRelaySell(54-1610)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Cloudflare Pages: pillarx-debug
- GitHub Check: build
🔇 Additional comments (1)
src/apps/pulse/hooks/useIntentSdk.ts (1)
201-214: Guard onintentSdkandpayingTokenslooks correctThe early return when
!intentSdk || !payingTokens?.lengthprevents undefinedchainIdaccess and addresses the previous empty-array risk aroundpayingTokens[0].
| let additionalTransactions: Transactions[] = []; | ||
| // Step 2: Install modules if not installed and not EIP-7702 eligible | ||
| if (!areModulesInstalled && !isEIP7702Eligible) { | ||
| try { | ||
| additionalTransactions = await getEnablePulseTradingTransactions(); | ||
| } catch (moduleErr) { | ||
| console.error('Module installation failed:', moduleErr); | ||
| setSetupError('Failed to install trading modules. Please try again.'); | ||
| setIsWaitingForSignature(false); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Step 3: Execute the top-up transaction | ||
| const userOperationHash = await executeTopUp({ | ||
| selectedToken, | ||
| amount, | ||
| allocateAmount, | ||
| sellOffer, | ||
| userPortfolio, | ||
| additionalTransactions, | ||
| }); | ||
|
|
||
| if (userOperationHash) { | ||
| setIsWaitingForSignature(false); | ||
| setIsLoading(false); | ||
|
|
||
| // Ensure we have a valid gas fee string for the transaction status | ||
| const gasFeeString = | ||
| gasCostNative && nativeTokenSymbol | ||
| ? `≈ ${formatExponentialSmallNumber(limitDigitsNumber(parseFloat(gasCostNative)))} ${nativeTokenSymbol}` | ||
| : '≈ 0.00'; | ||
|
|
||
| // Set transaction data and show transaction status | ||
| setUserOpHash(userOperationHash); | ||
| setTransactionGasFee(gasFeeString); | ||
| setCurrentTransactionStatus('Starting Transaction'); | ||
| setErrorDetails(''); | ||
| setSubmittedAt(undefined); | ||
| setPendingCompletedAt(undefined); | ||
| setBlockchainTxHash(undefined); | ||
| setHasSeenSuccess(false); | ||
| hasSeenSuccessRef.current = false; | ||
| blockchainTxHashRef.current = null; | ||
| failureGraceExpiryRef.current = null; | ||
| statusStartTime.current = Date.now(); | ||
| setShowTransactionStatus(true); | ||
| setIsPollingActive(true); | ||
|
|
||
| // Don't clear onboarding screen - keep showing transaction status within onboarding | ||
| return; | ||
| } | ||
|
|
||
| throw new Error('Failed to execute top-up transaction'); | ||
| } catch (err) { | ||
| console.error('Failed to execute top-up:', err); | ||
| // Check if the error is a user rejection | ||
| if ( | ||
| err instanceof Error && | ||
| err.message.includes('User rejected the request') | ||
| ) { | ||
| setIsTransactionRejected(true); | ||
| setIsWaitingForSignature(false); | ||
| } else { | ||
| // Other errors will be handled by the useTopUp hook's error state | ||
| setIsWaitingForSignature(false); | ||
| } | ||
| setIsLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Module-installation errors are effectively ignored; top-up may proceed without modules
In handleConfirm, the getEnablePulseTradingTransactions call is wrapped in a try/catch:
let additionalTransactions: Transactions[] = [];
if (!areModulesInstalled && !isEIP7702Eligible) {
try {
additionalTransactions = await getEnablePulseTradingTransactions();
} catch (moduleErr) {
// setSetupError + abort
}
}However, getEnablePulseTradingTransactions never throws; it logs and returns [] on failure (including when intentSdk or payingTokens are missing). That means:
- The
catchblock is effectively dead. - When modules are not installed and the helper fails, you still proceed to
executeTopUpwithadditionalTransactions = [], so the flow may run without module installation even thoughareModulesInstalled === false.
This can cause confusing failures deeper in the top-up flow instead of surfacing a clear “Failed to install trading modules” error.
Consider treating an empty result as a failure when modules are expected, e.g.:
let additionalTransactions: Transactions[] = [];
if (!areModulesInstalled && !isEIP7702Eligible) {
try {
- additionalTransactions = await getEnablePulseTradingTransactions();
+ additionalTransactions = await getEnablePulseTradingTransactions();
+ if (!additionalTransactions.length) {
+ throw new Error('No module installation transactions returned');
+ }
} catch (moduleErr) {
console.error('Module installation failed:', moduleErr);
setSetupError('Failed to install trading modules. Please try again.');
setIsWaitingForSignature(false);
setIsLoading(false);
return;
}
}Alternatively, have getEnablePulseTradingTransactions throw on real errors instead of returning [], so the existing try/catch works as intended.
🤖 Prompt for AI Agents
In src/apps/pulse/components/Onboarding/PreviewTopUp.tsx around lines 201-271,
the code assumes getEnablePulseTradingTransactions will throw on failure but it
returns an empty array instead, so module-installation failures are ignored and
the flow proceeds; after awaiting getEnablePulseTradingTransactions, check if
areModulesInstalled was false and the returned additionalTransactions array is
empty (or missing required transactions) and treat that as an installation
failure by logging the error, calling setSetupError('Failed to install trading
modules. Please try again.'), setting setIsWaitingForSignature(false) and
setIsLoading(false) and returning early (or rethrowing) so the top-up does not
proceed without modules; alternatively, update getEnablePulseTradingTransactions
to throw on unrecoverable errors and keep the existing try/catch — choose one
approach and implement it consistently.
There was a problem hiding this comment.
@vignesha22 please check this one if CodeRabbit is right or not?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
from the getEnablePulseTradingTransactions it would test if the modules is installed or not and then only send which are all the modules to be installed so if there is no installation needed then it would respond with empty array so this is not a problem though we can add and see why the check made was false but as far as user transaction is concerned this wont affect their flow
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/apps/pulse/hooks/useTopUp.ts (1)
285-300: Expose more specific errors where safe instead of a generic fallbackIn the catch block, you log the underlying error but always surface
'Something went wrong. Please try again later'to the hook consumer. Many errors inbuildTopUpTransactionsalready have user-friendly copy (e.g. missing paymaster URL, unsupported USDC, invalid token price), but those are hidden behind this generic message.Consider selectively passing through known-safe messages (e.g. configuration or validation issues) to improve UX, while still masking low-level or sensitive failures.
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
139-189: Clear stalesellOfferon all early exits in the quote effectIn the quote-fetching effect, several early returns (
!isRelayInitialized, missingportfolioToken/price) don’t resetsellOffer, so a previous quote can linger when prerequisites are no longer met and the UI will treat it as valid.const fetchSellOffer = async () => { // Only fetch for non-USDC tokens if ( !selectedToken || isSelectedTokenUSDC() || !amount || parseFloat(amount) <= 0 ) { setSellOffer(null); return; } if (!isRelayInitialized) { + setSellOffer(null); return; } // Find the token in portfolio to get actual balance and price const portfolioToken = portfolioTokens.find( (token) => token.contract.toLowerCase() === selectedToken.address.toLowerCase() ); if (!portfolioToken || !portfolioToken.balance || !portfolioToken.price) { + setSellOffer(null); return; }This keeps
sellOfferaligned with the current token, amount, and portfolio state.src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
203-215: Ensure module-installation failures don’t silently proceed
handleConfirmassumesgetEnablePulseTradingTransactionswill throw on failure, but if it still returns an empty array on error, the flow will continue without installing modules even whenareModulesInstalled === false.To be safe, treat an empty result as a failure when modules are expected:
let additionalTransactions: Transactions[] = []; // Step 2: Install modules if not installed and not EIP-7702 eligible if (!areModulesInstalled && !isEIP7702Eligible) { try { - additionalTransactions = await getEnablePulseTradingTransactions(); + additionalTransactions = await getEnablePulseTradingTransactions(); + if (!additionalTransactions.length) { + throw new Error('No module installation transactions returned'); + } } catch (moduleErr) {Or ensure
getEnablePulseTradingTransactionsnow throws on unrecoverable errors so thistry/catchis effective.
🧹 Nitpick comments (4)
src/apps/pulse/hooks/useTopUp.ts (1)
60-156: Type thetransactionsarray to avoidanyand ensure consistency
transactionsis currently an untyped array that mixes sell and paymaster deposit transactions. This makes it easy to push malformed objects and only discover issues at runtime when adding to the batch.Consider introducing a dedicated type (e.g.
TopUpTransaction) and using it consistently:-interface Transactions { - action: string; - calldata: string; - target: string; -} +interface Transactions { + action: string; + calldata: string; + target: string; +} + +interface TopUpTransaction { + chainId: number; + to: string; + value: string; + data: string; +} ... - const transactions = []; + const transactions: TopUpTransaction[] = [];This will also give you stronger guarantees that
tx.chainId,tx.to,tx.value, andtx.dataexist when batching.src/apps/pulse/components/Onboarding/TopUpScreen.tsx (2)
92-105: Reset amount and errors on token change to avoid stale stateWhen
selectedTokenchanges, you updateallocateAmountbut keep the previousamount,error, andsellOffer. That can leave a non-zero USD amount and old validation/quote tied to the prior token.Consider also resetting these when the token changes:
useEffect(() => { if (selectedToken && portfolioTokens.length > 0) { ... if (portfolioToken && portfolioToken.balance && portfolioToken.price) { const tokenBalanceUsd = portfolioToken.balance * portfolioToken.price; setAllocateAmount(parseFloat(tokenBalanceUsd.toFixed(2))); } } + setAmount(''); + setError(null); + setSellOffer(null); }, [selectedToken, portfolioTokens]);This mirrors the behavior in the Sell flow where inputs are cleared on token switch so liquidity/validation state doesn’t go stale. Based on learnings, this keeps onboarding consistent with existing UX.
515-541: Avoid recomputinggetSelectedTokenBalance()multiple times in renderWithin the wallet balance display you call
getSelectedTokenBalance()up to three times, each doing a portfolio lookup. This is minor but noisy and slightly wasteful.You can compute it once and reuse:
- {selectedToken ? ( - <> - {getSelectedTokenBalance().balance.toFixed(4)}{' '} - {getSelectedTokenBalance().symbol} - <span className="text-white/50"> - {' '} - (${getSelectedTokenBalance().usdValue.toFixed(2)}) - </span> - </> - ) : ( + {selectedToken ? (() => { + const { balance, symbol, usdValue } = getSelectedTokenBalance(); + return ( + <> + {balance.toFixed(4)} {symbol} + <span className="text-white/50"> + {' '}(${usdValue.toFixed(2)}) + </span> + </> + ); + })() : ( `$${initialBalance.toFixed(2)}` )}src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
543-558: Clarify swap step description to avoid mixing USD and token unitsThe “Swap to USDC” step currently uses:
description: `$${parseFloat(amount).toFixed(2)} ${selectedToken.symbol} for ${usdcReceived.toFixed(2)} USDC`,This reads like “$10 ETH”, which conflates USD and token units.
Consider explicitly labeling it as USD value or showing the token amount:
- description: `$${parseFloat(amount).toFixed(2)} ${selectedToken.symbol} for ${usdcReceived.toFixed(2)} USDC`, + description: `$${parseFloat(amount).toFixed(2)} worth of ${selectedToken.symbol} for ${usdcReceived.toFixed(2)} USDC`,or compute and display the token quantity similar to
TopUpScreenfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apps/pulse/assets/arrow-down-onboarding.svgis excluded by!**/*.svgsrc/apps/pulse/assets/back-arrow.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)src/apps/pulse/hooks/useGasTankBalance.ts(1 hunks)src/apps/pulse/hooks/useTopUp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/hooks/useGasTankBalance.ts
🧰 Additional context used
🧠 Learnings (3)
📚 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/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.tsx
📚 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/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.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/hooks/useTopUp.ts
🧬 Code graph analysis (1)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (1)
PortfolioToken(31-34)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
PreviewTopUp(75-775)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)
⏰ 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). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
| // Step 4: Deposit to Gas Tank | ||
| const depositAmount = isSelectedTokenUSDC() | ||
| ? parseFloat(amount) | ||
| : allocateAmount; | ||
| steps.push({ | ||
| icon: ( | ||
| <img src={OnboardingGasTankIcon} alt="Gas Tank" className="w-9 h-9" /> | ||
| ), | ||
| title: 'Deposit to Gas Tank', | ||
| description: `$${depositAmount.toFixed(2)} USDC into Gas Tank`, | ||
| bgColor: 'bg-[rgba(255,54,108,0.1)]', |
There was a problem hiding this comment.
Use allocateAmount for USDC deposit step to reflect actual top-up
For USDC tokens, the deposit step uses parseFloat(amount) while the actual on-chain deposit uses allocateAmount (which factors in the USDC price). If USDC deviates from $1, the previewed $X USDC into Gas Tank may not match the real deposit.
- const depositAmount = isSelectedTokenUSDC()
- ? parseFloat(amount)
- : allocateAmount;
+ const depositAmount = allocateAmount;This keeps the preview aligned with the amount actually sent to the gas tank for both USDC and non-USDC paths.
📝 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.
| // Step 4: Deposit to Gas Tank | |
| const depositAmount = isSelectedTokenUSDC() | |
| ? parseFloat(amount) | |
| : allocateAmount; | |
| steps.push({ | |
| icon: ( | |
| <img src={OnboardingGasTankIcon} alt="Gas Tank" className="w-9 h-9" /> | |
| ), | |
| title: 'Deposit to Gas Tank', | |
| description: `$${depositAmount.toFixed(2)} USDC into Gas Tank`, | |
| bgColor: 'bg-[rgba(255,54,108,0.1)]', | |
| // Step 4: Deposit to Gas Tank | |
| const depositAmount = allocateAmount; | |
| steps.push({ | |
| icon: ( | |
| <img src={OnboardingGasTankIcon} alt="Gas Tank" className="w-9 h-9" /> | |
| ), | |
| title: 'Deposit to Gas Tank', | |
| description: `$${depositAmount.toFixed(2)} USDC into Gas Tank`, | |
| bgColor: 'bg-[rgba(255,54,108,0.1)]', |
🤖 Prompt for AI Agents
In src/apps/pulse/components/Onboarding/PreviewTopUp.tsx around lines 561 to
571, the depositAmount is set to parseFloat(amount) when USDC is selected which
causes the preview to mismatch the actual on-chain deposit that uses
allocateAmount; update the assignment so the USDC branch uses allocateAmount
(i.e., replace the ternary to use allocateAmount for USDC instead of
parseFloat(amount) or simply set depositAmount = allocateAmount) so the
previewed “$X USDC into Gas Tank” matches the real deposit.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (2)
203-215: Module-installation errors are still effectively ignored.The
getEnablePulseTradingTransactionsfunction returns an empty array on failure rather than throwing. This means thecatchblock (lines 208-214) is effectively unreachable, and when modules are not installed but the helper fails, the flow proceeds withadditionalTransactions = [].Consider treating an empty result as a failure when modules are expected:
let additionalTransactions: Transactions[] = []; // Step 2: Install modules if not installed and not EIP-7702 eligible if (!areModulesInstalled && !isEIP7702Eligible) { try { additionalTransactions = await getEnablePulseTradingTransactions(); + if (additionalTransactions.length === 0) { + throw new Error('No module installation transactions returned'); + } } catch (moduleErr) { console.error('Module installation failed:', moduleErr); setSetupError('Failed to install trading modules. Please try again.'); setIsWaitingForSignature(false); setIsLoading(false); return; } }
561-564: Preview may mismatch actual deposit for USDC tokens.For USDC tokens,
depositAmountusesparseFloat(amount)while the actual on-chain deposit usesallocateAmount. If USDC deviates from $1, the previewed amount won't match the real deposit.Consider using
allocateAmountconsistently:// Step 4: Deposit to Gas Tank - const depositAmount = isSelectedTokenUSDC() - ? parseFloat(amount) - : allocateAmount; + const depositAmount = allocateAmount;
🧹 Nitpick comments (3)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
652-653: Consider using stable keys for step items.Using array index as key (
key={index}) is acceptable here since steps are computed fresh each render, but consider using a more descriptive key based on step title for better debugging in React DevTools:- {steps.map((step, index) => ( - <div key={index}> + {steps.map((step, index) => ( + <div key={step.title}>src/apps/pulse/components/App/HomeScreen.tsx (2)
568-570: Unnecessary dependency inshowTransactionStatuscallback.
setOnboardingScreenwas added to the dependency array, but it's not used within theshowTransactionStatuscallback body. This could cause unnecessary re-creation of the callback. If this was added to satisfy a lint rule, consider if it's actually needed.// eslint-disable-next-line react-hooks/exhaustive-deps - [sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy]
247-247: Consider extracting magic number for onboarding threshold.The value
2(gas tank balance threshold) is hardcoded in multiple places (lines 247, 249, 1076). Consider extracting it to a constant for maintainability.// In constants file or at top of component const MIN_GAS_TANK_BALANCE_FOR_TRADING = 2; // Then use throughout if (gasTankBalance < MIN_GAS_TANK_BALANCE_FOR_TRADING && onboardingScreen === null) {Also applies to: 1076-1076
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/Onboarding/PreviewTopUp.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/Onboarding/PreviewTopUp.tsxsrc/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
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/App/HomeScreen.tsx (7)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-98)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(13-102)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(34-670)
⏰ 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: Cloudflare Pages: pillarx-debug
- GitHub Check: build
🔇 Additional comments (12)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (5)
1-43: LGTM!Imports and interface definitions are well-organized with clear separation of concerns (hooks, types, utils, constants, components, services, assets).
44-73: LGTM!Type definitions are well-structured. The
UserOpStatustype covers all expected status values, and the props interface correctly types optional parameters.
117-130: VerifyactualBalandtotalRawhardcoded values.The
actualBalandtotalRaware hardcoded to'0'with a comment that balance is managed in the parent. Confirm thatuseIntentSdkdoesn't rely on these values for any calculations or validations that could affect the top-up flow.
388-395: LGTM!The cleanup effect properly clears the failure timeout on unmount, addressing the potential memory leak concern.
397-485: LGTM!The polling effect is well-implemented with proper cleanup. The use of refs (
hasSeenSuccessRef,blockchainTxHashRef,failureGraceExpiryRef) correctly avoids stale closure issues, and the intentional exclusion of certain dependencies is documented.src/apps/pulse/components/App/HomeScreen.tsx (7)
37-38: LGTM!New imports for onboarding components, hooks, and constants are properly organized and follow the existing file structure.
Also applies to: 46-47, 53-54
97-119: LGTM!The extended props interface and destructuring are well-typed and follow the existing component patterns.
220-237: LGTM!The onboarding state management is well-implemented. Using a ref for
hasCompletedOnboardingwith localStorage persistence is appropriate for this use case, and the new balance hooks are cleanly integrated.
240-261: Onboarding may repeatedly trigger after completion.The effect shows onboarding when
gasTankBalance < 2regardless of whether the user previously completed it. Per the discussion in past comments, this appears intentional ("should repeat when gas tank balance is less than $2 in order to top up again").However,
hasCompletedOnboardingRefis set inmarkOnboardingCompletebut not checked here. This means the ref and localStorage persistence at lines 1118-1120 don't prevent re-triggering. If the current behavior is intentional, consider removing the unused persistence logic or documenting the intent more clearly.
263-309: LGTM!The USDC token preselection logic is well-implemented. It correctly filters stable currencies, finds the one with the highest USD balance, and converts to the expected format. The guard at line 281 prevents the reduce from failing on an empty array.
311-326: LGTM!The token sync effect and navigation handlers are clean and focused. The sync correctly resets the
isSearchingFromTopupflag after token selection.
1103-1124: LGTM!The
TopUpScreenintegration is well-implemented. ThesetSearchingcallback correctly configures the search mode for top-up, andmarkOnboardingCompleteproperly persists the completion state.
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
520-585: Deposit-step text can drift from actual top-up amount (re-iterating prior comment)For the “Deposit to Gas Tank” step,
depositAmountis:const depositAmount = isSelectedTokenUSDC() ? parseFloat(amount) : allocateAmount;In the non‑USDC path, on‑chain deposit uses
allocateAmount(from the quote), so the step description matches reality. For USDC,allocateAmountis derived fromamountand the USDC price inTopUpScreen; if USDC ≠ $1.00,parseFloat(amount)andallocateAmountdiverge, so the preview text may not reflect the actual deposit.You can keep the preview consistent with the executed top‑up by always using
allocateAmount:- const depositAmount = isSelectedTokenUSDC() - ? parseFloat(amount) - : allocateAmount; + const depositAmount = allocateAmount;This reuses the same normalized “USDC to deposit” value for both display and execution.
🧹 Nitpick comments (2)
src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx (1)
7-187: Solid coverage; only minor test ergonomics nitsThe suite exercises layout, formatting, loading state, interactions, and accessibility quite well and aligns with the component implementation. If you find these tests growing further, consider extracting a small
renderOnboardingWelcome(overrides?)helper so you don’t repeatrender(<OnboardingWelcome {...mockProps} />)and so per-test prop overrides are clearer, but this is optional and not blocking.src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
91-105: Consider resettingamount/ validation when token changesWhen
selectedTokenchanges you recomputeallocateAmountfrom the new portfolio balance, butamountanderrorremain from the previous token. If the new token has a lower USD balance, the UI will immediately show “Insufficient balance” or similar until the user clears the field.Consider also resetting
amount(and clearingerror) in this effect when the token changes to avoid carrying over stale inputs/validation between tokens, similar to how the Sell flow resets the amount on token switch. Based on learnings, this keeps validation state in sync with the active token.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx(1 hunks)src/apps/pulse/hooks/useTopUp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/hooks/useTopUp.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.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/Onboarding/PreviewTopUp.tsxsrc/apps/pulse/components/Onboarding/TopUpScreen.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx (1)
src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(13-102)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (4)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (2)
139-205: Nice: stalesellOffercleared on all early exitsThe sell-offer effect now clears
sellOfferandisLoadingQuoteon every early return (no token,USDC,no amount,!isRelayInitialized, missingportfolioTokenorprice). That prevents a stale quote from sticking around when prerequisites aren’t met and addresses the earlier concern about outdated offers driving UI state.
639-668: I'm unable to complete the verification due to a repository clone failure. Let me explain what I need to verify this review comment properly:Information needed to verify the review comment:
Confirm
useRelaySellusage scope: DoesuseRelaySellhook only execute/set errors when a non-USDC token is selected? Or does it run unconditionally?Token switching behavior: When a user switches from a non-USDC token (with a
relayErrorset) to USDC, does therelayErrorstate automatically reset? (The learnings show that in Sell.tsx, input amount resets when tokens change—need to know if TopUpScreen has similar token-switching cleanup)Current button disabled logic: Verify that the actual code at lines 639-668 matches the snippet provided in the review comment.
Error state lifecycle: When/where is
relayErrorcleared? Is it:
- Cleared when token selection changes?
- Only cleared when a new quote is successfully fetched?
- Never cleared until component unmounts?
What I can assess from the provided context:
- The logical concern in the review comment is sound: if
relayErroris set by a non-USDC flow, it shouldn't block a pure-USDC transaction.- The suggested fix is architecturally correct: gating
relayErrorwithin!isSelectedTokenUSDC()condition prevents stale error state from blocking USDC flows.- The learnings support this pattern: similar token-switching logic exists in Sell.tsx.
Would you be able to provide:
- Access to the repository, or
- Confirmation of the behavior patterns above, or
- Output from reading the TopUpScreen.tsx file directly?
This will allow me to complete the verification and generate the final rewritten review comment.
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (2)
141-163: Gas estimation guard now avoids non‑null assertion riskPassing a dummy token object to
useGasEstimationTopUpand gating withisPaused: isWaitingForSignature || isLoading || !selectedTokenremoves the previousselectedToken!non‑null assertion and keeps the hook from running until real data is available. This is a clean way to satisfy the hook’s typing without risking a runtime crash.
294-405: Timeout and polling cleanup look robust
updateStatusWithDelaynow tracks the normal transition timeout vianormalTimeoutIdRef, and the cleanup effect cancels both failure and normal timeouts on unmount. Combined with theisPollingActiveguard and interval cleanup in the polling effect, this should avoid status updates after unmount and matches the earlier review feedback.
| const handleUsdAmountChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const { value } = e.target; | ||
| // Only allow numbers and decimals | ||
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | ||
| // Get selected token balance in USD | ||
| const { usdValue } = getSelectedTokenBalance(); | ||
| const numValue = parseFloat(value) || 0; | ||
|
|
||
| // Don't allow more than the token's USD balance | ||
| if (numValue > usdValue && value !== '') { | ||
| setError( | ||
| `Amount cannot exceed your balance of $${usdValue.toFixed(2)}` | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setAmount(value); | ||
| setError(null); // Clear error when user types | ||
| } | ||
| }; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-use-before-define | ||
| const handleQuickAmountClick = (item: string) => { | ||
| if (item === 'MAX') { | ||
| // Use selected token's USD balance as max | ||
| const { usdValue } = getSelectedTokenBalance(); | ||
| setAmount(usdValue.toFixed(2)); | ||
| } else { | ||
| setAmount(item); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Manual input blocked when no token is selected (UX inconsistency)
handleUsdAmountChange compares against getSelectedTokenBalance().usdValue. When no token is selected this is 0, so any non-empty positive input immediately trips the “Amount cannot exceed your balance of $0.00” path and effectively prevents typing, while the quick-amount buttons happily set a positive amount that is later rejected in handleTopUp.
If you want users to be able to type an amount before choosing a token, consider skipping the numValue > usdValue check when !selectedToken, or switching the error in that case to “Please select a token” so the message matches what’s actually wrong.
🤖 Prompt for AI Agents
In src/apps/pulse/components/Onboarding/TopUpScreen.tsx around lines 221 to 251,
the USD input handler compares against getSelectedTokenBalance().usdValue which
is 0 when no token is selected, blocking any positive manual input; update
handleUsdAmountChange to first check whether a token is selected (e.g. if
(!selectedToken) { setError('Please select a token'); setAmount(value); return;
} OR skip the usdValue check when selectedToken is falsy) so typing is not
rejected, and also guard handleQuickAmountClick MAX handling to only use
usdValue when selectedToken exists (otherwise set an appropriate "Please select
a token" error or prevent setting the amount). Ensure setError is cleared
appropriately when a token is selected and input is valid.
- Ensure closeTransactionStatus properly closes the onboarding flow on transaction success by calling setOnboardingScreen(null) - For failed transactions, return to preview screen via onBack() to allow retry - Fix OnboardingWelcome test to match actual description text without "and enable trading" suffix ✅ Build passes ✅ All onboarding tests pass (42 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
587-599: Previewed deposit amount for USDC can diverge from actual on-chain top-up
depositAmountusesparseFloat(amount)for USDC butallocateAmountfor non‑USDC, while the actual deposit logic (in the hooks) usesallocateAmountfor both. If USDC deviates from $1, the “$X USDC into Gas Tank” preview may not match the real deposit.Since the team is already comfortable with float arithmetic for these flows, aligning the preview with
allocateAmountis a small but important UX fix. Based on learnings, …- const depositAmount = isSelectedTokenUSDC() - ? parseFloat(amount) - : allocateAmount; + const depositAmount = allocateAmount;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx(1 hunks)src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Onboarding/tests/OnboardingWelcome.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/Onboarding/PreviewTopUp.tsx
📚 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/Onboarding/PreviewTopUp.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/Onboarding/PreviewTopUp.tsx
⏰ 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: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (1)
src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
420-501: Transaction polling and status mapping look solidThe
UserOpStatus→TransactionStatusStatemapping plus the polling loop and timing guards (min display time, failure grace window,hasSeenSuccessRef, cleanup of intervals/timeouts) are well thought out and should handle most backend status quirks gracefully.
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
639-639: Unused dependency in callback.The
showTransactionStatuscallback (lines 595-640) doesn't callsetOnboardingScreen, so it shouldn't be in the dependency array at line 639. This was flagged in a previous review but appears to still be present.Apply this diff:
[sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy]You can also remove the eslint-disable comment on line 638 if this is the only violation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/App/HomeScreen.tsx(11 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
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-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/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 (4)
src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-99)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(20-128)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(39-706)
⏰ 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: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (3)
src/apps/pulse/components/App/HomeScreen.tsx (3)
652-666: Good transaction cleanup logic.The conditional reset of
buyTokenbased onUSE_RELAY_BUYand unconditional reset ofsellTokenmakes sense for different transaction flows. Refetching the gas tank balance after successful transactions ensures the onboarding state stays accurate.
1153-1190: Clean onboarding screen rendering.The conditional rendering of
OnboardingWelcomeandTopUpScreenis well-structured. The distinction between onboarding initiated from settings (showCloseButton={isTopUpFromSettings}) versus automatic onboarding is a good UX detail.
267-285: Onboarding flow logic is well-implemented.The three effects work together to create a smooth onboarding experience:
- Show welcome screen when gas tank balance is insufficient (lines 267-285)
- Auto-dismiss with a 3-second delay when balance is restored (lines 287-302)
- Reset the completion flag when balance drops below threshold to re-trigger onboarding (lines 304-318)
The dependency arrays are complete and the logic handles edge cases appropriately.
Also applies to: 287-302, 304-318
| // Preselect max USDC token when topup screen is shown and totalUsdcBalance > 0 | ||
| useEffect(() => { | ||
| if ( | ||
| onboardingScreen === 'topup' && | ||
| totalUsdcBalance > 0 && | ||
| !topupToken && | ||
| walletPortfolioData?.result?.data?.assets | ||
| ) { | ||
| // Find all USDC tokens from the portfolio | ||
| const usdcTokens = portfolioTokens.filter((token) => { | ||
| const chainId = chainNameToChainIdTokensData(token.blockchain); | ||
| return STABLE_CURRENCIES.some( | ||
| (stable) => | ||
| stable.chainId === chainId && | ||
| stable.address.toLowerCase() === token.contract.toLowerCase() | ||
| ); | ||
| }); | ||
|
|
||
| if (usdcTokens.length > 0) { | ||
| // Find the USDC token with the highest USD balance | ||
| const maxUsdcToken = usdcTokens.reduce((max, token) => { | ||
| const currentBalance = (token.price || 0) * (token.balance || 0); | ||
| const maxBalance = (max.price || 0) * (max.balance || 0); | ||
| return currentBalance > maxBalance ? token : max; | ||
| }); | ||
|
|
||
| if ((maxUsdcToken.balance ?? 0) > 2) { | ||
| // Convert to SelectedToken format | ||
| setTopupToken({ | ||
| name: maxUsdcToken.name, | ||
| symbol: maxUsdcToken.symbol, | ||
| logo: maxUsdcToken.logo, | ||
| usdValue: String(maxUsdcToken.price || 0), | ||
| dailyPriceChange: 0, | ||
| chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain), | ||
| decimals: maxUsdcToken.decimals, | ||
| address: maxUsdcToken.contract, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }, [ | ||
| onboardingScreen, | ||
| totalUsdcBalance, | ||
| topupToken, | ||
| walletPortfolioData, | ||
| portfolioTokens, | ||
| setTopupToken, | ||
| ]); |
There was a problem hiding this comment.
Potential type mismatch with chainId.
At line 354, chainNameToChainIdTokensData(maxUsdcToken.blockchain) returns number | undefined (based on the function signature in relevant snippets), but SelectedToken.chainId expects number. If the blockchain name is unrecognized, this could cause a type error.
Consider adding a type guard or default value:
+ const chainId = chainNameToChainIdTokensData(maxUsdcToken.blockchain);
+ if (!chainId) {
+ return; // Skip if chainId cannot be determined
+ }
+
if ((maxUsdcToken.balance ?? 0) > 2) {
// Convert to SelectedToken format
setTopupToken({
name: maxUsdcToken.name,
symbol: maxUsdcToken.symbol,
logo: maxUsdcToken.logo,
usdValue: String(maxUsdcToken.price || 0),
dailyPriceChange: 0,
- chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain),
+ chainId,
decimals: maxUsdcToken.decimals,
address: maxUsdcToken.contract,
});
}🤖 Prompt for AI Agents
In src/apps/pulse/components/App/HomeScreen.tsx around lines 320 to 368, the
call to chainNameToChainIdTokensData(maxUsdcToken.blockchain) can return number
| undefined but SelectedToken.chainId expects number; guard against undefined by
resolving chainId into a local const, check if it's a valid number before
calling setTopupToken, and either skip selecting that token (return/continue) or
provide a safe default numeric chainId; ensure types align by narrowing the
union so that setTopupToken always receives a numeric chainId.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/apps/pulse/components/App/HomeScreen.tsx (2)
638-639: Remove unusedsetOnboardingScreenfromshowTransactionStatusdependencies.
showTransactionStatusdoes not referencesetOnboardingScreen, but it’s included in theuseCallbackdependency array:const showTransactionStatus = useCallback( (userOperationHash: string, gasFee?: string) => { // ... }, // eslint-disable-next-line react-hooks/exhaustive-deps - [sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy] );Keeping unused dependencies makes the hook signature misleading and can cause unnecessary invalidations if the setter identity ever changes. Since
setOnboardingScreenisn’t used, it’s safe to drop it and then also remove the eslint-disable comment if no longer needed.
316-354: GuardchainNameToChainIdTokensDatawhen settingtopupToken.chainId.In the USDC preselection effect,
chainNameToChainIdTokensDatais used both for filtering and to populateSelectedToken.chainId:const chainId = chainNameToChainIdTokensData(token.blockchain); // ... chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain),Since
chainNameToChainIdTokensDatacan returnnumber | undefined, an unrecognized blockchain name would yieldundefined, which violates theSelectedToken.chainId: numbercontract and can leak into downstream logic.Consider narrowing before setting
topupToken:const usdcTokens = portfolioTokens.filter((token) => { - const chainId = chainNameToChainIdTokensData(token.blockchain); - return STABLE_CURRENCIES.some( + const chainId = chainNameToChainIdTokensData(token.blockchain); + if (!chainId) return false; + return STABLE_CURRENCIES.some( (stable) => stable.chainId === chainId && stable.address.toLowerCase() === token.contract.toLowerCase() ); }); // ... - if ((maxUsdcToken.balance ?? 0) > 2) { - // Convert to SelectedToken format - setTopupToken({ + const chainId = chainNameToChainIdTokensData(maxUsdcToken.blockchain); + if (!chainId || (maxUsdcToken.balance ?? 0) <= 2) { + return; + } + + // Convert to SelectedToken format + setTopupToken({ name: maxUsdcToken.name, symbol: maxUsdcToken.symbol, logo: maxUsdcToken.logo, usdValue: String(maxUsdcToken.price || 0), dailyPriceChange: 0, - chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain), + chainId, decimals: maxUsdcToken.decimals, address: maxUsdcToken.contract, }); - }This mirrors a previous suggestion and keeps
SelectedTokentyping consistent.src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
242-262: Manual USD input is effectively blocked when no token is selected.
handleUsdAmountChangealways compares againstgetSelectedTokenBalance().usdValue, which is0whenselectedTokenisnull. Any positive input then triggers the “Amount cannot exceed your balance of $0.00” error and prevents typing, while quick-amount buttons can still set a positive amount. This was previously flagged and remains.Consider skipping the balance check when no token is selected or switching the error to “Please select a token” in that case, e.g.:
const handleUsdAmountChange = (e: React.ChangeEvent<HTMLInputElement>) => { const { value } = e.target; // Only allow numbers and decimals if (value === '' || /^\d*\.?\d*$/.test(value)) { - // Get selected token balance in USD - const { usdValue } = getSelectedTokenBalance(); - const numValue = parseFloat(value) || 0; - - // Don't allow more than the token's USD balance - if (numValue > usdValue && value !== '') { - setError( - `Amount cannot exceed your balance of $${usdValue.toFixed(2)}` - ); - return; - } + const { usdValue } = getSelectedTokenBalance(); + const numValue = parseFloat(value) || 0; + + if (!selectedToken && value !== '') { + setAmount(value); + setError('Please select a token'); + return; + } + + // Don't allow more than the token's USD balance + if (numValue > usdValue && value !== '') { + setError( + `Amount cannot exceed your balance of $${usdValue.toFixed(2)}` + ); + return; + }This keeps the UX consistent and avoids blocking typing when no token is selected.
🧹 Nitpick comments (3)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
65-69: Remove or gate debugconsole.logforshowCloseButton.The
useEffectat Lines 65–69 only logsshowCloseButtonand has no functional role. This should be removed or wrapped in a dev-only flag to avoid noisy console output in production.src/apps/pulse/components/App/HomeScreen.tsx (2)
248-265: Effect to “hide onboarding” is effectively a no-op.The effect:
useEffect(() => { if ( (buyToken || sellToken || previewBuy || previewSell) && onboardingScreen === null ) { setOnboardingScreen(null); } }, [...]);only calls
setOnboardingScreen(null)whenonboardingScreenis alreadynull, so it doesn’t actually hide anything.Either remove this effect, or change the condition to act when onboarding is showing, e.g.:
- if ( - (buyToken || sellToken || previewBuy || previewSell) && - onboardingScreen === null - ) { - setOnboardingScreen(null); - } + if ( + (buyToken || sellToken || previewBuy || previewSell) && + onboardingScreen !== null + ) { + setOnboardingScreen(null); + }to match the comment “Immediately hide onboarding when a token is selected or preview opens.”
375-385: Avoid permanentconsole.lognoise for onboarding/top-up handlers.
handleShowTopUpandhandleBackToWelcomelog messages on every invocation:console.log('[HomeScreen] handleShowTopUp called with fromSettings:', fromSettings); console.log('[HomeScreen] handleBackToWelcome called - resetting isTopUpFromSettings to false');These are useful during development but noisy in production. Consider removing them or gating behind a debug flag (e.g. a feature flag or
NODE_ENV !== 'production').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/pulse/components/App/HomeScreen.tsx(11 hunks)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx(1 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 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/Onboarding/TopUpScreen.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 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/Onboarding/TopUpScreen.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Onboarding/TopUpScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
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
⏰ 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: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (3)
src/apps/pulse/components/App/HomeScreen.tsx (3)
651-665: Token reset and gas tank refetch on transaction completion look correct.In
closeTransactionStatus, resettingbuyToken/sellTokenonly on'Transaction Complete'and then callingrefetchGasTankBalance()is a good integration point:
- It avoids clearing tokens on failed/cancelled transactions.
- It ensures gas tank onboarding/gating reacts promptly to successful top-ups and other transactions that might modify gas credit.
No changes needed here.
1151-1188: Onboarding render branches are wired coherently.The
renderPreviewbranches foronboardingScreen === 'welcome'and'topup'correctly:
- Pass gas/onboarding data (
gasTankBalance,isGasTankLoading,maxStableCoinBalance) intoOnboardingWelcome.- Route
onCompletetohandleShowTopUp, withfromSettingsexplicitly flagged when invoked fromSettingsMenu.- Provide
TopUpScreenwithinitialBalance={totalUsdcBalance},portfolioTokens, onboarding setters, andshowCloseButton={isTopUpFromSettings}to differentiate settings-triggered flows.This arrangement matches the described feature behavior and keeps onboarding concerns local to
renderPreview.
1274-1284: Settings “Top Up” entry point is correctly integrated.Wiring
onTopUp={() => handleShowTopUp(true)}withgasTankBalanceintoSettingsMenucleanly exposes the same onboarding/top-up flow from settings without duplicating logic. This is a sensible reuse of the existing handlers.
| const handleQuickAmountClick = (item: string) => { | ||
| const percentage = parseInt(item.replace('%', ''), 10); | ||
| const { usdValue } = getSelectedTokenBalance(); | ||
|
|
||
| if (usdValue > 0) { | ||
| const calculatedAmount = (usdValue * percentage) / 100; | ||
| // Round to 2 decimals | ||
| setAmount(calculatedAmount.toFixed(2)); | ||
| } else { | ||
| setAmount('0'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Quick-amount buttons should also clear any existing error state.
handleQuickAmountClick updates amount based on the selected percentage but never clears error. If the user previously hit a validation error (e.g. exceeded balance via manual input), the error persists and the Top Up button remains disabled even after picking a valid quick amount.
Add setError(null) after setting the amount:
const handleQuickAmountClick = (item: string) => {
const percentage = parseInt(item.replace('%', ''), 10);
const { usdValue } = getSelectedTokenBalance();
if (usdValue > 0) {
const calculatedAmount = (usdValue * percentage) / 100;
// Round to 2 decimals
- setAmount(calculatedAmount.toFixed(2));
+ setAmount(calculatedAmount.toFixed(2));
+ setError(null);
} else {
setAmount('0');
+ setError(null);
}
};📝 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.
| const handleQuickAmountClick = (item: string) => { | |
| const percentage = parseInt(item.replace('%', ''), 10); | |
| const { usdValue } = getSelectedTokenBalance(); | |
| if (usdValue > 0) { | |
| const calculatedAmount = (usdValue * percentage) / 100; | |
| // Round to 2 decimals | |
| setAmount(calculatedAmount.toFixed(2)); | |
| } else { | |
| setAmount('0'); | |
| } | |
| }; | |
| const handleQuickAmountClick = (item: string) => { | |
| const percentage = parseInt(item.replace('%', ''), 10); | |
| const { usdValue } = getSelectedTokenBalance(); | |
| if (usdValue > 0) { | |
| const calculatedAmount = (usdValue * percentage) / 100; | |
| // Round to 2 decimals | |
| setAmount(calculatedAmount.toFixed(2)); | |
| setError(null); | |
| } else { | |
| setAmount('0'); | |
| setError(null); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/apps/pulse/components/Onboarding/TopUpScreen.tsx around lines 265 to 276,
handleQuickAmountClick sets the calculated amount but does not clear any
existing validation error, so a prior error can persist and keep the Top Up
button disabled; after setting the amount (both when using
calculatedAmount.toFixed(2) and when setting '0'), call setError(null) to clear
the error state so the UI re-validates and enables the button when the new
amount is valid.
| const handleAllocateAmountChange = ( | ||
| e: React.ChangeEvent<HTMLInputElement> | ||
| ) => { | ||
| const { value } = e.target; | ||
| // Only allow numbers and decimals | ||
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | ||
| const numValue = parseFloat(value) || 0; | ||
|
|
||
| // For non-USDC tokens, max is the received USDC amount from sell offer | ||
| let maxAmount: number; | ||
| if (!isSelectedTokenUSDC() && sellOffer) { | ||
| maxAmount = sellOffer.tokenAmountToReceive; | ||
| } else { | ||
| maxAmount = parseFloat(amount) || 0; | ||
| } | ||
|
|
||
| // Don't allow more than the max amount | ||
| if (numValue <= maxAmount || value === '') { | ||
| setAllocateAmount(numValue); | ||
| setError(null); // Clear error when user types | ||
| } else { | ||
| setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleAllocateDecrease = () => { | ||
| const minAmount = 2; // Minimum 2 USDC for gas tank | ||
| if (allocateAmount > minAmount) { | ||
| setAllocateAmount(Math.max(minAmount, allocateAmount - 1)); | ||
| } | ||
| }; | ||
|
|
||
| const handleAllocateIncrease = () => { | ||
| // For non-USDC tokens, max is the received USDC amount from sell offer | ||
| let maxAmount: number; | ||
| if (!isSelectedTokenUSDC() && sellOffer) { | ||
| maxAmount = sellOffer.tokenAmountToReceive; | ||
| } else { | ||
| maxAmount = parseFloat(amount) || 0; | ||
| } | ||
|
|
||
| if (allocateAmount < maxAmount) { | ||
| setAllocateAmount(Math.min(maxAmount, allocateAmount + 1)); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
“Minimum 2 USDC” comment is not enforced in validation.
handleAllocateDecrease enforces a minimum of 2 USDC only when using the − button, but:
handleAllocateAmountChangeallows manual values< 2with no error.handleTopUpdoes not enforce any minimum onallocateAmountoramount.
Given the inline comment “Minimum 2 USDC for gas tank”, either:
- Enforce a true minimum (for non‑USDC flows) in
handleAllocateAmountChange/handleTopUp, or - Adjust the comment to avoid implying a protocol constraint that isn’t actually enforced.
Example enforcing a minimum for non‑USDC flows:
const handleAllocateAmountChange = (
e: React.ChangeEvent<HTMLInputElement>
) => {
const { value } = e.target;
// Only allow numbers and decimals
if (value === '' || /^\d*\.?\d*$/.test(value)) {
const numValue = parseFloat(value) || 0;
+ const minAmount = 2;
// For non-USDC tokens, max is the received USDC amount from sell offer
let maxAmount: number;
if (!isSelectedTokenUSDC() && sellOffer) {
maxAmount = sellOffer.tokenAmountToReceive;
} else {
maxAmount = parseFloat(amount) || 0;
}
- // Don't allow more than the max amount
- if (numValue <= maxAmount || value === '') {
+ // Don't allow less than min or more than max (when applicable)
+ if ((value === '' || numValue >= minAmount) && numValue <= maxAmount) {
setAllocateAmount(numValue);
setError(null); // Clear error when user types
} else {
- setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`);
+ if (numValue < minAmount) {
+ setError(`Allocate amount must be at least ${minAmount.toFixed(2)} USDC`);
+ } else {
+ setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`);
+ }
}
}
};Or, if no true min is required, update the comment to avoid confusion.
📝 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.
| const handleAllocateAmountChange = ( | |
| e: React.ChangeEvent<HTMLInputElement> | |
| ) => { | |
| const { value } = e.target; | |
| // Only allow numbers and decimals | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| const numValue = parseFloat(value) || 0; | |
| // For non-USDC tokens, max is the received USDC amount from sell offer | |
| let maxAmount: number; | |
| if (!isSelectedTokenUSDC() && sellOffer) { | |
| maxAmount = sellOffer.tokenAmountToReceive; | |
| } else { | |
| maxAmount = parseFloat(amount) || 0; | |
| } | |
| // Don't allow more than the max amount | |
| if (numValue <= maxAmount || value === '') { | |
| setAllocateAmount(numValue); | |
| setError(null); // Clear error when user types | |
| } else { | |
| setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`); | |
| } | |
| } | |
| }; | |
| const handleAllocateDecrease = () => { | |
| const minAmount = 2; // Minimum 2 USDC for gas tank | |
| if (allocateAmount > minAmount) { | |
| setAllocateAmount(Math.max(minAmount, allocateAmount - 1)); | |
| } | |
| }; | |
| const handleAllocateIncrease = () => { | |
| // For non-USDC tokens, max is the received USDC amount from sell offer | |
| let maxAmount: number; | |
| if (!isSelectedTokenUSDC() && sellOffer) { | |
| maxAmount = sellOffer.tokenAmountToReceive; | |
| } else { | |
| maxAmount = parseFloat(amount) || 0; | |
| } | |
| if (allocateAmount < maxAmount) { | |
| setAllocateAmount(Math.min(maxAmount, allocateAmount + 1)); | |
| } | |
| }; | |
| const handleAllocateAmountChange = ( | |
| e: React.ChangeEvent<HTMLInputElement> | |
| ) => { | |
| const { value } = e.target; | |
| // Only allow numbers and decimals | |
| if (value === '' || /^\d*\.?\d*$/.test(value)) { | |
| const numValue = parseFloat(value) || 0; | |
| const minAmount = 2; | |
| // For non-USDC tokens, max is the received USDC amount from sell offer | |
| let maxAmount: number; | |
| if (!isSelectedTokenUSDC() && sellOffer) { | |
| maxAmount = sellOffer.tokenAmountToReceive; | |
| } else { | |
| maxAmount = parseFloat(amount) || 0; | |
| } | |
| // Don't allow less than min or more than max (when applicable) | |
| if ((value === '' || numValue >= minAmount) && numValue <= maxAmount) { | |
| setAllocateAmount(numValue); | |
| setError(null); // Clear error when user types | |
| } else { | |
| if (numValue < minAmount) { | |
| setError(`Allocate amount must be at least ${minAmount.toFixed(2)} USDC`); | |
| } else { | |
| setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`); | |
| } | |
| } | |
| } | |
| }; | |
| const handleAllocateDecrease = () => { | |
| const minAmount = 2; // Minimum 2 USDC for gas tank | |
| if (allocateAmount > minAmount) { | |
| setAllocateAmount(Math.max(minAmount, allocateAmount - 1)); | |
| } | |
| }; | |
| const handleAllocateIncrease = () => { | |
| // For non-USDC tokens, max is the received USDC amount from sell offer | |
| let maxAmount: number; | |
| if (!isSelectedTokenUSDC() && sellOffer) { | |
| maxAmount = sellOffer.tokenAmountToReceive; | |
| } else { | |
| maxAmount = parseFloat(amount) || 0; | |
| } | |
| if (allocateAmount < maxAmount) { | |
| setAllocateAmount(Math.min(maxAmount, allocateAmount + 1)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/apps/pulse/components/Onboarding/TopUpScreen.tsx around lines 279-325,
the comment "Minimum 2 USDC for gas tank" is misleading because manual input and
handleTopUp allow values < 2; enforce the minimum consistently: define a
minAmount (2) where you compute maxAmount, then in handleAllocateAmountChange
clamp or reject values below minAmount (setAllocateAmount to minAmount or
setError describing the minimum) and in handleAllocateIncrease/Decrease keep
current behavior but ensure they respect minAmount; additionally add a
validation in handleTopUp to block/normalize allocateAmount < minAmount before
submitting (or update the comment if you decide not to enforce a protocol
minimum). Ensure error messages and clamping only apply for non‑USDC flows if
that restriction is intended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (2)
273-305: Minimum 2 USDC constraint is only enforced on the decrease button.The comment at line 301 states "Minimum 2 USDC for gas tank", but this is only enforced in
handleAllocateDecrease. Manual input viahandleAllocateAmountChangeand the finalhandleTopUpvalidation allow values below 2 USDC.Either enforce the minimum consistently across all input paths, or remove/update the comment to avoid implying a protocol constraint that isn't enforced.
const handleAllocateAmountChange = ( e: React.ChangeEvent<HTMLInputElement> ) => { const { value } = e.target; // Only allow numbers and decimals if (value === '' || /^\d*\.?\d*$/.test(value)) { const numValue = parseFloat(value) || 0; + const minAmount = 2; // Minimum 2 USDC for gas tank // For non-USDC tokens, max is the received USDC amount from sell offer let maxAmount: number; if (!isSelectedTokenUSDC() && sellOffer) { maxAmount = sellOffer.tokenAmountToReceive; } else { maxAmount = parseFloat(amount) || 0; } // Don't allow more than the max amount - if (numValue <= maxAmount || value === '') { + if ((numValue >= minAmount || value === '') && numValue <= maxAmount) { setAllocateAmount(numValue); setError(null); + } else if (numValue < minAmount && value !== '') { + setError(`Allocate amount must be at least ${minAmount} USDC`); } else { setError(`Allocate amount cannot exceed ${maxAmount.toFixed(2)} USDC`); } } };
259-271: Quick-amount buttons should clear any existing error state.
handleQuickAmountClickupdatesamountbut never clearserror. If the user previously hit a validation error, the error persists and the Top Up button remains disabled even after picking a valid quick amount.const handleQuickAmountClick = (item: string) => { const percentage = parseInt(item.replace('%', ''), 10); const { usdValue } = getSelectedTokenBalance(); if (usdValue > 0) { const calculatedAmount = (usdValue * percentage) / 100; // Round to 2 decimals setAmount(calculatedAmount.toFixed(2)); + setError(null); } else { setAmount('0'); + setError(null); } };src/apps/pulse/components/App/HomeScreen.tsx (2)
304-314:hasCompletedOnboardingflag can oscillate between true/false while balance stays ≥ 2.When
gasTankBalance >= 2and!hasCompletedOnboardingRef.currentis true, the flag is set to true. On the next effect run (still withgasTankBalance >= 2),!hasCompletedOnboardingRef.currentis now false, so theelsebranch executes and resets the flag to false.This causes the localStorage and ref to flip on every balance change, potentially re-triggering onboarding incorrectly.
useEffect(() => { - if (gasTankBalance >= 2 && !hasCompletedOnboardingRef.current) { - hasCompletedOnboardingRef.current = true; - localStorage.setItem('hasCompletedOnboarding', 'true'); - } else { - hasCompletedOnboardingRef.current = false; - localStorage.setItem('hasCompletedOnboarding', 'false'); + if (gasTankBalance >= 2) { + if (!hasCompletedOnboardingRef.current) { + hasCompletedOnboardingRef.current = true; + localStorage.setItem('hasCompletedOnboarding', 'true'); + } + } else { + if (hasCompletedOnboardingRef.current) { + hasCompletedOnboardingRef.current = false; + localStorage.setItem('hasCompletedOnboarding', 'false'); + } } }, [gasTankBalance]);
342-354: Guard against invalidchainIdwhen preselecting USDC token.
chainNameToChainIdTokensDatareturns0for unrecognized blockchain names. SettingchainId: 0on theSelectedTokenmay cause downstream issues if a valid chain ID is expected.+ const chainId = chainNameToChainIdTokensData(maxUsdcToken.blockchain); + if (!chainId) { + return; // Skip if chainId cannot be determined + } + if ((maxUsdcToken.balance ?? 0) > 2) { // Convert to SelectedToken format setTopupToken({ name: maxUsdcToken.name, symbol: maxUsdcToken.symbol, logo: maxUsdcToken.logo, usdValue: String(maxUsdcToken.price || 0), dailyPriceChange: 0, - chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain), + chainId, decimals: maxUsdcToken.decimals, address: maxUsdcToken.contract, }); }
🧹 Nitpick comments (4)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (2)
224-235: Consider reusingchainIdToChainNameTokensDatainstead of duplicating chain name mapping.This local
getChainNamefunction duplicates the chain ID to name mapping that already exists intokensData.ts. The implementations also differ slightly (e.g., this returns "BSC" while the external function returns "BNB Smart Chain (BEP20)").- // Get chain name from chain ID - const getChainName = (chainId: number): string => { - const chainNames: { [key: number]: string } = { - 1: 'Ethereum', - 137: 'Polygon', - 10: 'Optimism', - 42161: 'Arbitrum', - 8453: 'Base', - 56: 'BSC', - }; - return chainNames[chainId] || 'Unknown'; - };Then use
chainIdToChainNameTokensData(selectedToken.chainId)at line 519. If you need shortened names for UI purposes, consider adding a display variant to the existing utility.
321-361: Consider adding minimum allocation validation inhandleTopUp.The validation checks for valid amount, selected token, and balance constraints, but doesn't validate that
allocateAmountmeets the minimum 2 USDC requirement for non-USDC tokens before navigating to preview. This could allow proceeding with invalid allocations if the user manually enters a value below 2.// Additional validation for non-USDC tokens if (!isSelectedTokenUSDC()) { if (!sellOffer) { return; } + if (allocateAmount < 2) { + setError('Allocate amount must be at least 2 USDC'); + return; + } + const maxAllocateAmount = sellOffer.tokenAmountToReceive; if (allocateAmount > maxAllocateAmount) { setError( `Allocate amount cannot exceed ${maxAllocateAmount.toFixed(2)} USDC` ); return; } }src/apps/pulse/components/App/HomeScreen.tsx (2)
250-264: This effect is a no-op and can be removed.The condition
onboardingScreen === nullmeanssetOnboardingScreen(null)is called when it's alreadynull. This effect doesn't change any state.If the intent was to hide onboarding when a token/preview is active and onboarding is currently showing, the condition should check for non-null onboarding states:
useEffect(() => { if ( - (buyToken || sellToken || previewBuy || previewSell) && - onboardingScreen === null + (buyToken || sellToken || previewBuy || previewSell) && + onboardingScreen !== null ) { setOnboardingScreen(null); } }, [However, the original past review comment indicated this was intentionally gated to not interfere with active onboarding flows, so this may have been addressed by design. Please verify the intended behavior.
636-636: Remove unusedsetOnboardingScreenfrom dependency array.
setOnboardingScreenis not called withinshowTransactionStatus(lines 592-634). Removing it avoids unnecessary callback invalidations.// eslint-disable-next-line react-hooks/exhaustive-deps - [sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/App/HomeScreen.tsx(11 hunks)src/apps/pulse/components/Onboarding/TopUpScreen.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/Onboarding/TopUpScreen.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 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/Onboarding/TopUpScreen.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
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 (2)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (5)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/services/tokensData.ts (2)
PortfolioToken(31-34)chainIdToChainNameTokensData(210-231)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/PreviewTopUp.tsx (1)
PreviewTopUp(75-815)src/utils/blockchain.ts (1)
getLogoForChainId(165-199)
src/apps/pulse/components/App/HomeScreen.tsx (7)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-99)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(20-128)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(39-706)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (11)
src/apps/pulse/components/Onboarding/TopUpScreen.tsx (5)
1-63: LGTM on imports, interface, and initial setup.The imports are well-organized, the props interface is clearly defined, and the initial state setup follows React conventions with appropriate defaults.
65-94: LGTM on helper functions.
isSelectedTokenUSDCandgetSelectedTokenBalanceare correctly implemented with proper null checks and fallback values.
152-222: StalesellOfferhandling is now addressed.The effect now properly clears
sellOfferandisLoadingQuoteon all early-exit branches, preventing stale quote data from persisting when prerequisites aren't met.
367-427: LGTM on button text logic.The
getButtonTextfunction handles all states appropriately: loading states, USDC vs non-USDC tokens, and swap quote display.
669-702: LGTM on button disabled logic.The disabled condition correctly accounts for all validation states: empty/zero amount, loading states, missing sell offer for non-USDC tokens, and error conditions.
src/apps/pulse/components/App/HomeScreen.tsx (6)
37-55: LGTM on new imports.The onboarding-related imports are well-organized and correctly categorized by their purpose (components, hooks, constants).
96-121: LGTM on extended props and destructuring.The new onboarding-related props are properly typed and destructured.
240-246: LGTM on onboarding data hooks.The
useGasTankBalanceanduseTotalUsdcBalancehooks are correctly integrated with proper null handling foraccountAddress.
648-663: LGTM on post-transaction cleanup.The logic correctly resets tokens after successful transaction completion and refetches gas tank balance. The conditional reset based on
USE_RELAY_BUYand transaction type is appropriate.
1149-1186: LGTM on onboarding screen rendering.The
renderPreviewfunction correctly handles the onboarding flow states and passes appropriate props toOnboardingWelcomeandTopUpScreencomponents.
1279-1280: LGTM on settings menu integration.The top-up action from settings is properly wired with
handleShowTopUp(true)and the gas tank balance is passed for display.
github-actions
bot
requested a deployment
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 0
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)
86-112: Critical: Undefined variableportfolioTokenswill cause runtime error.The
portfolioTokensvariable is referenced in theuseMemo(line 87) but is not defined anywhere in the component—it's not in props, state, or returned from any hook. Additionally,getChainId(line 92) andMobulaChainNames(line 92) are used but never imported. This will cause aReferenceErrorat runtime and break the PnL calculation feature.According to the AI summary,
portfolioTokenswas removed from the props as part of this refactor, but the dependent code was not updated.You need to either:
- Remove this
useMemoblock and theuseTokenPnLhook usage (lines 86-112) if portfolio-based PnL is no longer needed, OR- Restore the
portfolioTokensprop and add the missing imports forgetChainIdandMobulaChainNamesApply this diff if removing portfolio-based PnL:
- // Find matching portfolio token to get balance and price - const portfolioToken = useMemo(() => { - if (!token || !portfolioTokens || portfolioTokens.length === 0) return null; - - return portfolioTokens.find( - (pt) => - pt.contract.toLowerCase() === token.address.toLowerCase() && - Number(getChainId(pt.blockchain as MobulaChainNames)) === token.chainId - ); - }, [token, portfolioTokens]); - - // Calculate PnL for selected token with proper balance and price - const { pnl, isLoading: isPnLLoading } = useTokenPnL( - token && accountAddress && portfolioToken - ? { - token: { - contract: token.address || '', - symbol: token.symbol, - decimals: token.decimals || 18, - balance: portfolioToken.balance || 0, - price: portfolioToken.price || 0, - }, - transactionsData, - walletAddress: accountAddress, - chainId: token.chainId, - } - : null - ); + // Calculate PnL for selected token + const { pnl, isLoading: isPnLLoading } = useTokenPnL( + token && accountAddress + ? { + token: { + contract: token.address || '', + symbol: token.symbol, + decimals: token.decimals || 18, + balance: tokenBalance || 0, + price: parseFloat(token.usdValue) || 0, + }, + transactionsData, + walletAddress: accountAddress, + chainId: token.chainId, + } + : null + );Also remove the unused import:
-import { PortfolioToken } from '../../../../services/tokensData';
♻️ Duplicate comments (2)
src/apps/pulse/components/App/HomeScreen.tsx (2)
304-314: Onboarding completion flag oscillates on every balance update.The current logic in this effect will flip the
hasCompletedOnboardingflag betweentrueandfalsewhenevergasTankBalancechanges, even when the balance remains above the threshold:
- First update:
gasTankBalance = 2.5,ref.current = false→ setsref.current = true✓- Next update:
gasTankBalance = 2.6,ref.current = true→ hitselsebranch → setsref.current = false✗This was flagged in a previous review but appears unresolved.
Apply this diff to fix the oscillation:
useEffect(() => { - if (gasTankBalance >= 2 && !hasCompletedOnboardingRef.current) { - hasCompletedOnboardingRef.current = true; - localStorage.setItem('hasCompletedOnboarding', 'true'); - } else { - hasCompletedOnboardingRef.current = false; - localStorage.setItem('hasCompletedOnboarding', 'false'); - } + if (gasTankBalance >= 2) { + if (!hasCompletedOnboardingRef.current) { + hasCompletedOnboardingRef.current = true; + localStorage.setItem('hasCompletedOnboarding', 'true'); + } + } else if (gasTankBalance < 2) { + if (hasCompletedOnboardingRef.current) { + hasCompletedOnboardingRef.current = false; + localStorage.setItem('hasCompletedOnboarding', 'false'); + } + } }, [gasTankBalance]);
316-364: Guard against invalid chainId when preselecting USDC.At line 350,
chainNameToChainIdTokensData(maxUsdcToken.blockchain)can return0if the blockchain name is unrecognized. SettingchainId: 0creates an invalidSelectedTokenthat may cause downstream errors in swap/transaction logic.Apply this diff to validate the chainId before selecting the token:
if (usdcTokens.length > 0) { // Find the USDC token with the highest USD balance const maxUsdcToken = usdcTokens.reduce((max, token) => { const currentBalance = (token.price || 0) * (token.balance || 0); const maxBalance = (max.price || 0) * (max.balance || 0); return currentBalance > maxBalance ? token : max; }); + const chainId = chainNameToChainIdTokensData(maxUsdcToken.blockchain); + + // Only select token if we have a valid chainId and sufficient balance - if ((maxUsdcToken.balance ?? 0) > 2) { + if ((maxUsdcToken.balance ?? 0) > 2 && chainId > 0) { // Convert to SelectedToken format setTopupToken({ name: maxUsdcToken.name, symbol: maxUsdcToken.symbol, logo: maxUsdcToken.logo, usdValue: String(maxUsdcToken.price || 0), dailyPriceChange: 0, - chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain), + chainId, decimals: maxUsdcToken.decimals, address: maxUsdcToken.contract, }); } }
🧹 Nitpick comments (1)
src/apps/pulse/components/App/HomeScreen.tsx (1)
636-636: Remove unusedsetOnboardingScreenfrom dependency array.The
showTransactionStatuscallback (lines 592-637) does not referencesetOnboardingScreenanywhere in its body, so it should not be in the dependency array.Apply this diff:
}, // eslint-disable-next-line react-hooks/exhaustive-deps - [sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/apps/pulse/components/App/HomeScreen.tsx(11 hunks)src/apps/pulse/components/App/tests/AppWrapper.test.tsx(3 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(4 hunks)src/apps/pulse/components/Buy/Buy.tsx(2 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(3 hunks)src/apps/pulse/components/Search/Search.tsx(3 hunks)src/apps/pulse/components/Sell/Sell.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/apps/pulse/components/Search/PortfolioTokenList.tsx
- src/apps/pulse/components/App/tests/AppWrapper.test.tsx
- src/apps/pulse/components/Buy/Buy.tsx
- src/apps/pulse/components/App/tests/HomeScreen.test.tsx
- src/apps/pulse/components/Search/Search.tsx
- src/apps/pulse/components/Buy/BuyButton.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 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/Sell/Sell.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsx
📚 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/Sell/Sell.tsxsrc/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/Sell/Sell.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/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 (7)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-99)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(20-128)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(39-706)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (4)
src/apps/pulse/components/Sell/Sell.tsx (2)
476-493: LGTM: Error handling simplified correctly.The removal of
minGasAmountchecks and simplification of the error message aligns with the PR's refactoring goals. The error banner now appropriately shows when there's insufficient token balance or a relay error, without the native token balance requirement.
586-596: LGTM: SellButton prop correctly simplified.The
notEnoughLiquidityprop now correctly passes only the token balance check, consistent with the removal of gas/native token requirements from this component.src/apps/pulse/components/App/HomeScreen.tsx (2)
649-663: LGTM! Token reset and balance refetch logic.The transaction completion handling correctly:
- Resets tokens after successful transactions (respecting the
USE_RELAY_BUYflag for buy flows)- Refetches gas tank balance to update UI after deposits
1150-1186: LGTM! Onboarding UI integration.The onboarding screen rendering is well-structured:
- Welcome screen properly passes balance data and handlers
- TopUp screen correctly configures search mode (sell-only with stable coins included)
- Intentional design to defer marking onboarding complete until balance validation confirms the deposit
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/apps/pulse/components/Buy/BuyButton.tsx (1)
6-6: MovegetChainNameimport back under the utils sectionPrevious feedback on this line asked for
getChainNameto live with the other utils imports; please move this import down under the// utilsblock to align with that convention and close out the earlier comment.src/apps/pulse/components/App/HomeScreen.tsx (2)
638-638: Remove unusedsetOnboardingScreenfrom dependency array.The
showTransactionStatuscallback (lines 594–639) does not callsetOnboardingScreen, so it should not be in the dependency array at line 638. This was noted in a previous review and marked as addressed, but the dependency is still present.Apply this diff:
// eslint-disable-next-line react-hooks/exhaustive-deps - [sellToken, tokenAmount, sellOffer, isBuy, setOnboardingScreen] + [sellToken, tokenAmount, sellOffer, isBuy]
318-366: Guard against invalid chain ID when preselecting USDC.At line 352,
chainNameToChainIdTokensDatareturns0for unrecognized blockchain names. SettingchainId: 0inSelectedTokenmay cause issues in downstream code expecting valid chain IDs (e.g., RPC calls, chain-specific logic).Apply this diff to guard against invalid chain IDs:
+ const chainId = chainNameToChainIdTokensData(maxUsdcToken.blockchain); + if (!chainId || chainId === 0) { + // Skip tokens with unrecognized blockchains + return; + } + if ((maxUsdcToken.balance ?? 0) > 2) { // Convert to SelectedToken format setTopupToken({ name: maxUsdcToken.name, symbol: maxUsdcToken.symbol, logo: maxUsdcToken.logo, usdValue: String(maxUsdcToken.price || 0), dailyPriceChange: 0, - chainId: chainNameToChainIdTokensData(maxUsdcToken.blockchain), + chainId, decimals: maxUsdcToken.decimals, address: maxUsdcToken.contract, }); }This ensures only tokens with recognized blockchains are preselected for top-up.
🧹 Nitpick comments (2)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
89-95: Consider removing the redundant initial sort.The sort by USD value at lines 91–95 is immediately overwritten by the main sorting logic at lines 106–138, which applies
sortConfig(price/balance/pnl). This initial sort adds unnecessary computation without affecting the final output.Apply this diff to remove the redundant sort:
- let tokens = convertPortfolioAPIResponseToToken(walletPortfolioData) - .filter((token) => includeStableCoins || !isStableCurrency(token)) - .sort((a: Token, b: Token) => { - const balanceUSDA = (a.price || 0) * (a.balance || 0); - const balanceUSDB = (b.price || 0) * (b.balance || 0); - return balanceUSDB - balanceUSDA; // Sort by highest USD value first - }); + let tokens = convertPortfolioAPIResponseToToken(walletPortfolioData) + .filter((token) => includeStableCoins || !isStableCurrency(token));The final sort order is determined by the sorting logic at lines 106–138 based on
sortConfig, making this early sort redundant.src/apps/pulse/components/App/HomeScreen.tsx (1)
250-264: No-op effect can be removed.The effect sets
onboardingScreentonullonly when it's alreadynull(line 253), making it a no-op. This check was likely added to prevent unmounting during the onboarding flow, but the result is that the effect serves no purpose.Apply this diff to remove the no-op effect:
- // Immediately hide onboarding when a token is selected or preview opens - // But only if not in an active onboarding flow (welcome or topup screen) - useEffect(() => { - if ( - (buyToken || sellToken || previewBuy || previewSell) && - onboardingScreen === null - ) { - setOnboardingScreen(null); - } - }, [ - buyToken, - sellToken, - previewBuy, - previewSell, - onboardingScreen, - setOnboardingScreen, - ]); -The onboarding visibility is already managed by other effects (lines 267–285 and 287–302), making this effect redundant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pulse/components/App/HomeScreen.tsx(11 hunks)src/apps/pulse/components/App/tests/AppWrapper.test.tsx(3 hunks)src/apps/pulse/components/App/tests/HomeScreen.test.tsx(4 hunks)src/apps/pulse/components/Buy/BuyButton.tsx(1 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(4 hunks)src/apps/pulse/components/Sell/Sell.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apps/pulse/components/Sell/Sell.tsx
- src/apps/pulse/components/App/tests/HomeScreen.test.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 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/tests/AppWrapper.test.tsxsrc/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/pulse/components/App/tests/AppWrapper.test.tsxsrc/apps/pulse/components/App/HomeScreen.tsxsrc/apps/pulse/components/Buy/BuyButton.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-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.tsxsrc/apps/pulse/components/Buy/BuyButton.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 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/App/HomeScreen.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Search/PortfolioTokenList.tsx
🧬 Code graph analysis (2)
src/apps/pulse/components/App/HomeScreen.tsx (7)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useGasTankBalance.ts (1)
useGasTankBalance(22-99)src/apps/pulse/hooks/useTotalUsdcBalance.ts (1)
useTotalUsdcBalance(10-35)src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)src/apps/pulse/constants/tokens.ts (1)
STABLE_CURRENCIES(13-15)src/apps/pulse/components/Onboarding/OnboardingWelcome.tsx (1)
OnboardingWelcome(20-128)src/apps/pulse/components/Onboarding/TopUpScreen.tsx (1)
TopUpScreen(39-706)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(27-61)src/services/tokensData.ts (1)
Token(19-29)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (15)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
38-38: LGTM: Clean prop addition with sensible default.The new
includeStableCoinsprop with a default value offalsepreserves the existing behavior while enabling stable coin visibility when needed during onboarding flows.Also applies to: 49-49
152-158: LGTM: Dependency array is complete and accurate.All dependencies used within
getFilteredPortfolioTokensare correctly included in the dependency array, including the newly addedincludeStableCoins.src/apps/pulse/components/App/tests/AppWrapper.test.tsx (2)
11-11: LGTM: Mock setup follows established patterns.The import and mock configuration for
useGasTankBalanceis consistent with other hooks mocked in this test file.Also applies to: 37-39
130-133: LGTM: Mock values ensure main interface rendering.Setting
totalBalance: 100(non-zero) ensures the main interface is rendered rather than the onboarding flow, which aligns with the test objectives for AppWrapper.src/apps/pulse/components/App/HomeScreen.tsx (11)
19-19: LGTM: Imports and prop additions are well-organized.The new imports for onboarding components, gas tank hooks, and stable currencies are correctly integrated. The extended
HomeScreenPropsinterface cleanly adds the necessary onboarding state management props.Also applies to: 36-37, 45-46, 54-55, 96-102
223-246: LGTM: State initialization includes persistence and proper hook usage.The onboarding completion tracking via
localStorageensures persistence across sessions, and the gas tank/USDC balance hooks are correctly initialized with appropriate parameters.
267-285: LGTM: Welcome screen trigger logic is correct.The effect correctly shows the welcome screen only when the gas tank balance is insufficient (
< 2) and portfolio data is available, addressing the requirement that onboarding should appear when the user has no gas tank balance.
287-302: LGTM: Auto-dismiss logic provides good UX.The 3-second delay before auto-dismissing the welcome screen when balance reaches the threshold gives users visual feedback that their top-up was successful, with proper timeout cleanup.
304-316: LGTM: Onboarding completion flag is now managed correctly.The revised logic prevents unnecessary flag toggling by only updating when the state actually changes, addressing the previous concern about the flag flipping between true/false on every balance update.
368-375: LGTM: Token synchronization for top-up flow is correct.The effect properly syncs the selected token from search to the top-up flow and resets the search-from-top-up flag after selection.
377-385: LGTM: Navigation handlers for onboarding flow are clean.The handlers correctly manage navigation between onboarding screens and track whether top-up was initiated from settings vs. the welcome flow.
651-665: LGTM: Transaction completion handling is correct.The logic appropriately resets tokens after successful transactions and refetches the gas tank balance to reflect the updated state, with proper conditional handling for Relay Buy mode.
1152-1188: LGTM: Onboarding screen rendering is well-integrated.Both
OnboardingWelcomeandTopUpScreenare rendered with appropriate props, and thesetSearchingcallback correctly configures the search mode for token selection during top-up.
1206-1206: LGTM: Gas tank balance propagated to preview components.Passing
gasTankBalanceto both preview components enables them to display accurate gas tank information during transaction previews.Also applies to: 1226-1226
1281-1282: LGTM: Settings menu integration with top-up flow.The settings menu now has access to the top-up handler and gas tank balance, enabling users to top up their gas tank from the settings screen.
github-actions
bot
temporarily deployed
to
Preview (PRO-3733-OnboardingFlow)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/apps/pulse/components/Transaction/TransactionDetails.tsx (1)
178-180: Consider renamingtokenAmountToReceiveto clarify it represents USDC, not token units.The variable name is misleading—despite its name suggesting a token quantity,
tokenAmountToReceiveconsistently represents the USDC amount received from token swaps. This is evident from usage like"Selling ${amount} ${token.symbol} for ${sellOffer.tokenAmountToReceive.toFixed(2)} USDC"(useRelaySell.ts line 921) and assignment tousdcAmount(PreviewSell.tsx line 563). Renaming tousdcAmountToReceivewould improve code clarity without affecting functionality.src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
167-174: Consider making the fetch mock more specific.The global
fetchmock intercepts all fetch calls and returns{ priceUSD: 1.5 }regardless of the URL. IfPreviewSellor its dependencies make other fetch calls expecting different responses, this could lead to test failures or false positives.Consider validating the URL and returning appropriate responses:
- // Mock fetch for native price endpoint - global.fetch = vi.fn(() => - Promise.resolve({ - ok: true, - json: () => Promise.resolve({ priceUSD: 1.5 }), - } as any) - ); + // Mock fetch for native price endpoint + global.fetch = vi.fn((url: string) => { + if (url.includes('native-price') || url.includes('price')) { + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ priceUSD: 1.5 }), + } as any); + } + return Promise.reject(new Error(`Unexpected fetch to ${url}`)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx(3 hunks)src/apps/pulse/components/Transaction/TransactionDetails.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/Transaction/TransactionDetails.tsx
📚 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/Transaction/TransactionDetails.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/pulse/components/Transaction/TransactionDetails.tsx
🧬 Code graph analysis (1)
src/apps/pulse/components/Transaction/TransactionDetails.tsx (1)
src/utils/number.tsx (1)
limitDigitsNumber(42-69)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (3)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (3)
176-178: LGTM!Adding
afterEachcleanup withvi.restoreAllMocks()is excellent test hygiene. This ensures each test runs in isolation and prevents mock pollution between tests.
245-252: LGTM!Simulating asynchronous behavior with a delay in the
executeSellmock makes the test more realistic and better validates async state transitions (e.g., showing the "Please open your wallet" message).
66-72: The test mock structure correctly reflects the component implementation.The
chainGroupsstructure change from array to object keyed by chain ID is intentional and correctly implemented. The component explicitly accesses it assentBatch?.chainGroups?.[sellToken.chainId]?.userOpHash(line 475), and the test mock at lines 66-72 matches this pattern withchainGroups: { 1: { userOpHash: '0x...' } }. ThetotalCostfield at batch level is also correctly placed and used to convert gas costs from wei to USD (lines 487-490).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
174-182: Consider adding test coverage for fetch error scenarios.The global fetch mock only covers the success case (returning
priceUSD: 1.5). If the PreviewSell component has error handling for failed price fetches, consider adding test cases to verify that behavior.Example test case structure:
it('handles native price fetch failure gracefully', async () => { global.fetch = vi.fn(() => Promise.reject(new Error('Network error')) ); renderWithProviders(); // Assert appropriate error handling or fallback behavior });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx(4 hunks)
⏰ 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). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (4)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (4)
153-159: LGTM! Good test isolation practice.Stubbing the paymaster URL in the test suite ensures predictable behavior and aligns with the PR's paymaster configuration changes.
184-186: LGTM! Improved test cleanup.Using
vi.restoreAllMocks()ensures thorough cleanup of all mocks between tests, including the newly added global fetch mock.
253-260: LGTM! Better async behavior simulation.Adding a delay before resolving the transaction hash more accurately simulates real-world async transaction execution and helps identify potential race conditions.
66-72: Chaingroups and totalCost structure in mock data is correct and consistent.The mock data correctly reflects the actual implementation:
chainGroupsis an object keyed by chainId (not an array), andtotalCostis properly placed at the batch level. This structure is consistently used across the codebase in useGasEstimation hooks, PreviewBuy, PreviewSell, and SendModal components, all accessing these fields with the expected patterns (chainGroups?.[chainId]?.userOpHashandsentBatch?.totalCost).
4 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Background Services
Search / Portfolio
Buy/Sell UX
Tests
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.