Conversation
WalkthroughAdds a Coinbase onramp "Add Cash" flow and UI; persists Pulse HomeScreen settings to localStorage; introduces cross‑chain sell/bridge logic and portfolio propagation; switches per‑chain userOpHash handling; adds a proxied Coinbase Pages Function, functions TypeScript config, Vite proxy and dev tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WalletPortfolioButtons
participant Backend as App Backend (JWT)
participant Proxy as /api/coinbase
participant Coinbase
participant Browser
User->>UI: Click "Add Cash"
UI->>UI: Validate accountAddress
alt valid
UI->>Backend: GET onramp JWT
Backend-->>UI: JWT
UI->>UI: Resolve client IP (best-effort)
UI->>Proxy: POST /api/coinbase (Auth: JWT, body includes destinationAddress, currencies, clientIp)
Proxy->>Coinbase: Forward POST
Coinbase-->>Proxy: { onramp_url }
Proxy-->>UI: { onramp_url }
UI->>Browser: open onramp_url in new tab
else invalid
UI->>User: show validation/error
end
sequenceDiagram
participant UI as Sell UI
participant Preview as PreviewSell
participant Hook as useRelaySell / useGasEstimation
participant RelaySDK
participant Chain as DestinationChain
UI->>Preview: Request sell preview (sellToken, toChainId, userPortfolio)
Preview->>Hook: getBestSellOffer or getBestSellOfferWithBridge
Hook->>RelaySDK: buildSellTransactions or buildSellTransactionWithBridge (passes userPortfolio)
RelaySDK-->>Hook: transactions + fee estimates
Hook-->>Preview: SellOffer + gas estimate
User->>UI: Confirm sell
UI->>Hook: executeSell(..., userPortfolio)
Hook->>RelaySDK: send transactions (returns sentBatch with chainGroups keyed by chainId)
Hook-->>UI: userOpHash per chain for polling
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-04T14:34:00.373ZApplied to files:
📚 Learning: 2025-08-20T09:14:16.888ZApplied to files:
🧬 Code graph analysis (1)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
⏰ 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)
🔇 Additional comments (2)
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 |
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
Deploying x with
|
| Latest commit: |
e041f21
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cae431c2.x-e62.pages.dev |
| Branch Preview URL: | https://feature-pro-3781-add-cash.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/apps/pulse/components/App/HomeScreen.tsx (2)
226-230: Remove debug console.logs.These debug logs should be removed before merging to production.
Apply this diff:
useEffect(() => { - console.log('Max stable coin balance chain ID:', maxStableCoinBalance); const storedChainId = localStorage.getItem( 'pulse_selectedChainIdForSettlement' ); - console.log('Stored chain ID for settlement:', storedChainId);
265-287: LGTM with optional suggestion.The localStorage persistence logic is correct and includes appropriate guards. These three effects could optionally be combined into a single effect for slightly better performance, but the current implementation is clear and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apps/pulse/components/App/tests/__snapshots__/AppWrapper.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/App/tests/__snapshots__/HomeScreen.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(3 hunks)src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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-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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
📚 Learning: 2025-08-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-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/pulse/components/App/HomeScreen.tsx
🧬 Code graph analysis (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppDispatch(5-5)src/hooks/useEIP7702Upgrade.tsx (1)
useEIP7702Upgrade(31-188)
🪛 Biome (2.1.2)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
[error] 19-19: Shouldn't redeclare 'useTransactionKit'. Consider to delete it or rename it.
'useTransactionKit' is defined here:
(lint/suspicious/noRedeclare)
[error] 121-121: This code is unreachable
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (6)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
145-153: LGTM!The Add Cash button is properly integrated and consistent with the existing button styling and structure.
src/apps/pulse/components/App/HomeScreen.tsx (5)
121-128: LGTM!The localStorage initialization for custom amounts includes sensible defaults and proper error handling via JSON parsing in a try-catch context (implicit in the arrow function).
132-135: LGTM!The localStorage initialization for settlement chain ID includes a sensible default and will be updated by the useEffect based on max stable balance.
1046-1048: Clarify the fallback balance value.The fallback
balance: 2seems arbitrary. Should this be0to represent no balance, or is there a specific reason for using2? Please clarify the intent behind this value.
956-956: LGTM!The padding removal aligns with the PR objective to fix mobile design.
110-110: No issues found—optional type handling is correct.The type change to optional is properly managed throughout the codebase:
- In HomeScreen, all accesses use optional chaining (
maxStableCoinBalance?.chainId) or conditional guards before accessing properties- The nullish coalescing fallback at lines 1046-1048 (
maxStableCoinBalance ?? { chainId: 1, balance: 2 }) ensures the Buy component always receives a defined object- Buy component's prop type contract (non-optional) is honored—it receives a concrete object, never undefined
- No unsafe property access exists anywhere in the codebase
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Outdated
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Outdated
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
115-147: Still need UX feedback & loading guard for Add Cash.Lines 115-147 still leave the happy-path only; when anything fails the user gets no feedback, and the button stays enabled while the network stack is busy. This was flagged previously and is still outstanding: we need a loading state to prevent duplicate session creation and a surfaced toast/modal so the user understands the failure. Please add a scoped
isAddCashLoadingflag (disable the button & show spinner copy) and route errors through the app’s toast mechanism instead of silently failing.Apply this diff:
+ const [isAddCashLoading, setIsAddCashLoading] = useState(false); + const handleAddCash = async () => { + if (isAddCashLoading) return; + setIsAddCashLoading(true); try { // Validate wallet address if (!accountAddress) { throw new Error('Wallet address is not available'); } @@ - window.open(onrampUrl, '_blank', 'noopener,noreferrer'); + window.open(onrampUrl, '_blank', 'noopener,noreferrer'); } catch (error) { - // TODO: Replace with toast notification or proper error UI - // For now, silently fail - user can try again - // Error is logged for debugging in development - if (process.env.NODE_ENV === 'development') { - // eslint-disable-next-line no-console - console.error('Error opening add cash URL:', error); - } + showToast({ + type: 'error', + title: 'Add Cash failed', + message: + error instanceof Error ? error.message : 'Please try again in a moment.', + }); + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.error('Error opening add cash URL:', error); + } + } finally { + setIsAddCashLoading(false); } }; @@ - <button + <button type="button" className="flex py-[9px] px-3 w-fit h-fit items-center justify-center border-x-2 border-t-2 border-b-4 rounded-[10px] border-[#121116] cursor-pointer" - onClick={() => handleAddCash()} + onClick={handleAddCash} + disabled={isAddCashLoading} > <div className="flex gap-2 items-center justify-center rounded-lg cursor-pointer"> - <BodySmall>Add Cash</BodySmall> + <BodySmall>{isAddCashLoading ? 'Opening…' : 'Add Cash'}</BodySmall> </div> </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(4 hunks)src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-08-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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/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-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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
| const [customBuyAmounts, setCustomBuyAmounts] = useState<string[]>(() => { | ||
| const stored = localStorage.getItem('pulse_customBuyAmounts'); | ||
| return stored ? JSON.parse(stored) : ['10', '20', '50', '100']; | ||
| }); | ||
| const [customSellAmounts, setCustomSellAmounts] = useState<string[]>(() => { | ||
| const stored = localStorage.getItem('pulse_customSellAmounts'); | ||
| return stored ? JSON.parse(stored) : ['10%', '25%', '50%', '75%']; | ||
| }); | ||
| const [displaySettingsMenu, setDisplaySettingsMenu] = | ||
| useState<boolean>(false); | ||
| const [selectedChainIdForSettlement, setSelectedChainIdForSettlement] = | ||
| useState<number>(maxStableCoinBalance.chainId); | ||
| useState<number>(() => { | ||
| const stored = localStorage.getItem('pulse_selectedChainIdForSettlement'); | ||
| return stored ? parseInt(stored, 10) : 1; // Will be updated by useEffect once maxStableCoinBalance is calculated | ||
| }); | ||
| const [payingTokens, setPayingTokens] = useState<PayingToken[]>([]); |
There was a problem hiding this comment.
Defensively parse persisted settings to avoid crashes.
Lines 121, 126, and 132 read from localStorage with JSON.parse / parseInt and assume happy-path data. If a user (or a previous build) wrote malformed JSON or a non-numeric chain id, JSON.parse will throw or parseInt will return NaN, which breaks the entire HomeScreen (and even propagates NaN into settlement requests). Please harden these initializers with try/catch and fall back to sane defaults.
Apply this diff:
const [customBuyAmounts, setCustomBuyAmounts] = useState<string[]>(() => {
- const stored = localStorage.getItem('pulse_customBuyAmounts');
- return stored ? JSON.parse(stored) : ['10', '20', '50', '100'];
+ const stored = localStorage.getItem('pulse_customBuyAmounts');
+ if (!stored) return ['10', '20', '50', '100'];
+ try {
+ const parsed = JSON.parse(stored);
+ return Array.isArray(parsed) ? parsed : ['10', '20', '50', '100'];
+ } catch {
+ return ['10', '20', '50', '100'];
+ }
});
const [customSellAmounts, setCustomSellAmounts] = useState<string[]>(() => {
- const stored = localStorage.getItem('pulse_customSellAmounts');
- return stored ? JSON.parse(stored) : ['10%', '25%', '50%', '75%'];
+ const stored = localStorage.getItem('pulse_customSellAmounts');
+ if (!stored) return ['10%', '25%', '50%', '75%'];
+ try {
+ const parsed = JSON.parse(stored);
+ return Array.isArray(parsed) ? parsed : ['10%', '25%', '50%', '75%'];
+ } catch {
+ return ['10%', '25%', '50%', '75%'];
+ }
});
const [displaySettingsMenu, setDisplaySettingsMenu] =
useState<boolean>(false);
const [selectedChainIdForSettlement, setSelectedChainIdForSettlement] =
useState<number>(() => {
const stored = localStorage.getItem('pulse_selectedChainIdForSettlement');
- return stored ? parseInt(stored, 10) : 1; // Will be updated by useEffect once maxStableCoinBalance is calculated
+ if (!stored) return 1;
+ const parsed = Number.parseInt(stored, 10);
+ return Number.isNaN(parsed) ? 1 : parsed;
});🤖 Prompt for AI Agents
In src/apps/pulse/components/App/HomeScreen.tsx around lines 121-136, the
localStorage initializers assume well-formed data which can throw or produce
NaN; wrap each read/parse in a try/catch and validate types: for
customBuyAmounts and customSellAmounts try to JSON.parse but on any error or if
result is not an array, fall back to the respective default arrays and coerce
entries to strings (Array.isArray check and map to String); for
selectedChainIdForSettlement read the raw value, attempt parseInt inside try,
and if the result is NaN or not a finite integer fall back to 1; ensure these
safe defaults are returned from the initializer so malformed persisted values
don’t crash the component or produce NaN chain ids.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vite.config.js (1)
32-39: Consider adding error handling and logging to the proxy configuration.The proxy configuration is correctly set up with
changeOrigin,rewrite, andsecureoptions. However, adding error handling and logging callbacks would improve debugging during development.Apply this diff to enhance the proxy with error handling:
proxy: { '/api/coinbase': { target: 'https://api.cdp.coinbase.com', changeOrigin: true, rewrite: (path) => path.replace(/^\/api\/coinbase/, ''), secure: true, + configure: (proxy, options) => { + proxy.on('error', (err, req, res) => { + console.error('Proxy error:', err); + }); + proxy.on('proxyReq', (proxyReq, req, res) => { + console.log('Proxying:', req.method, req.url, '→', proxyReq.path); + }); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vite.config.js(1 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). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
107-113: Check response status before parsing JSON.Line 107 calls
response.json()unconditionally, which will fail for non-2xx status codes or empty responses. When the fetch fails, the error is caught and silently suppressed (lines 116-124), leaving the user with no feedback. This was flagged in a previous review and remains unresolved.Apply this diff:
); + if (!response.ok) { + const errorBody = await response.text().catch(() => ''); + throw new Error( + `Failed to create onramp session (${response.status}): ${errorBody}` + ); + } + const data = await response.json(); const onrampUrl = data?.session?.onrampUrl;
🧹 Nitpick comments (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
76-85: Consider a more useful IP fallback.While the best-effort IP lookup is reasonable, falling back to
127.0.0.1(localhost) may not be useful for the Coinbase API, which likely needs a real IP address for compliance. Consider using an empty string or omitting theclientIpfield entirely when unavailable, allowing the backend/proxy to handle it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(4 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vite.config.js
🧰 Additional context used
🧠 Learnings (2)
📚 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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (4)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (4)
7-7: LGTM! Clean import and usage.The duplicate import issue has been resolved, and the destructuring of
walletAddressasaccountAddressis appropriate for the Coinbase onramp integration context.Also applies to: 23-23
44-52: LGTM! Address validation is sound.The validation correctly checks for the presence of the wallet address and verifies it matches the Ethereum address format.
54-73: LGTM! Token fetching handles errors appropriately.The backend token retrieval correctly checks
response.okbefore parsing and validates the token exists.
140-148: LGTM! Button UI is consistent.The Add Cash button follows the same styling pattern as the existing Receive and Upgrade buttons, maintaining visual consistency.
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
requested a deployment
to
Preview (feature/PRO-3781-Add_Cash)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
vite.config.js (1)
6-6: The basicSsl plugin remains unused with https disabled.This issue was previously flagged. The
basicSsl()plugin (lines 6, 9) generates SSL certificates for HTTPS, but line 30 disables HTTPS withhttps: false, making the plugin serve no purpose.Also applies to: 9-9, 30-30
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
88-114: Check response.ok before parsing JSON.This issue was previously flagged and remains unresolved. Line 106 calls
response.json()unconditionally, which will throw on non-2xx responses or empty bodies, causing the error to be silently caught and logged only in development.Apply this diff:
const response = await fetch( '/api/coinbase/platform/v2/onramp/sessions', { method: 'POST', headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json', }, body: JSON.stringify({ purchaseCurrency: 'USDC', destinationNetwork: 'base', destinationAddress: accountAddress, paymentCurrency: 'USD', clientIp, }), } ); + if (!response.ok) { + const errorBody = await response.text().catch(() => ''); + throw new Error( + `Failed to create onramp session (${response.status}): ${errorBody}` + ); + } + const data = await response.json(); const onrampUrl = data?.session?.onrampUrl; if (!onrampUrl) { throw new Error('No URL received from Coinbase API'); } window.open(onrampUrl, '_blank', 'noopener,noreferrer');
41-124: Add loading state and user feedback.This issue was previously flagged and remains unresolved. The async operation provides no visual feedback, the button can be clicked multiple times during execution, and errors fail silently in production.
Consider:
- Adding a loading state to disable the button during processing
- Using a toast notification or modal for error feedback
- Showing success feedback when the URL opens
🧹 Nitpick comments (2)
functions/api/coinbase/platform/v2/onramp/sessions.js (1)
37-40: Consider tightening CORS policy.The
Access-Control-Allow-Origin: '*'header allows any origin to call this endpoint. While the endpoint still requires a valid Bearer token, consider restricting the allowed origins to your known frontend domains for defense in depth.Example:
const allowedOrigins = [ 'https://your-app.com', 'https://staging.your-app.com', 'http://localhost:5173', // Vite dev server ]; const origin = context.request.headers.get('Origin'); const allowOrigin = allowedOrigins.includes(origin) ? origin : allowedOrigins[0]; // Then use allowOrigin instead of '*' in the response headers 'Access-Control-Allow-Origin': allowOrigin,Also applies to: 58-61
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
75-85: Consider server-side IP detection for reliability and privacy.The code fetches the client IP from a third-party service (
api.ipify.org). This introduces:
- External dependency that could fail or be blocked
- Privacy concern (client makes additional request to third party)
- The fallback
127.0.0.1is likely not useful to Coinbase's fraud detectionConsider detecting the client IP on your backend (in the token endpoint or the Cloudflare function) where you have access to request headers like
CF-Connecting-IPorX-Forwarded-For. This would be more reliable and privacy-friendly.Example in the Cloudflare function:
const clientIp = context.request.headers.get('CF-Connecting-IP') || context.request.headers.get('X-Forwarded-For') || '127.0.0.1';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
functions/api/coinbase/platform/v2/onramp/sessions.js(1 hunks)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(4 hunks)vite.config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
🧬 Code graph analysis (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
functions/api/coinbase/platform/v2/onramp/sessions.js (2)
response(20-27)data(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (4)
vite.config.js (1)
32-39: LGTM on the proxy configuration.The proxy setup correctly routes
/api/coinbaserequests to Coinbase's API with appropriate options:changeOriginfor host header forwarding, path rewriting to strip the prefix, andsecure: truefor SSL validation.functions/api/coinbase/platform/v2/onramp/sessions.js (1)
54-64: LGTM on CORS preflight handler.The
onRequestOptionsfunction correctly handles CORS preflight requests with a 204 status and appropriate headers. The 24-hour cache (Access-Control-Max-Age: 86400) is reasonable.src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
44-52: LGTM on address validation.The validation correctly checks for both the existence of
accountAddressand its format using the standard Ethereum address regex pattern.
139-147: LGTM on button UI implementation.The Add Cash button is styled consistently with the existing Receive and Upgrade buttons and properly wired to the
handleAddCashhandler.
| if (!authHeader) { | ||
| return new Response(JSON.stringify({ error: 'Missing Authorization header' }), { | ||
| status: 401, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add CORS headers to 401 response.
The 401 response is missing CORS headers, which will cause the browser to block the response even though it returned correctly from the server. This makes debugging auth failures difficult for the client.
Apply this diff:
if (!authHeader) {
return new Response(JSON.stringify({ error: 'Missing Authorization header' }), {
status: 401,
- headers: { 'Content-Type': 'application/json' },
+ headers: {
+ 'Content-Type': 'application/json',
+ 'Access-Control-Allow-Origin': '*',
+ 'Access-Control-Allow-Methods': 'POST, OPTIONS',
+ 'Access-Control-Allow-Headers': 'Content-Type, Authorization',
+ },
});
}📝 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.
| if (!authHeader) { | |
| return new Response(JSON.stringify({ error: 'Missing Authorization header' }), { | |
| status: 401, | |
| headers: { 'Content-Type': 'application/json' }, | |
| }); | |
| } | |
| if (!authHeader) { | |
| return new Response(JSON.stringify({ error: 'Missing Authorization header' }), { | |
| status: 401, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*', | |
| 'Access-Control-Allow-Methods': 'POST, OPTIONS', | |
| 'Access-Control-Allow-Headers': 'Content-Type, Authorization', | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In functions/api/coinbase/platform/v2/onramp/sessions.js around lines 12 to 17,
the 401 response for a missing Authorization header lacks CORS headers; update
the returned Response to include the same CORS headers used for successful
responses (e.g., Access-Control-Allow-Origin, Access-Control-Allow-Methods,
Access-Control-Allow-Headers and any other project-standard CORS headers) so
browsers will accept the error response; ensure the headers object in the
Response contains those CORS entries and keep the JSON body and 401 status
unchanged.
| // Get the response data | ||
| const data = await response.json(); |
There was a problem hiding this comment.
Guard JSON parsing of Coinbase response.
Calling response.json() without checking the response status or content-type could throw if Coinbase returns an error, empty body, or non-JSON content. This will cause the catch block to return a generic 500 error, hiding the actual Coinbase error details.
Apply this diff:
- // Get the response data
- const data = await response.json();
+ // Get the response data
+ if (!response.ok) {
+ const errorText = await response.text().catch(() => 'Unknown error');
+ return new Response(
+ JSON.stringify({ error: `Coinbase API error (${response.status}): ${errorText}` }),
+ {
+ status: response.status,
+ headers: {
+ 'Content-Type': 'application/json',
+ 'Access-Control-Allow-Origin': '*',
+ },
+ }
+ );
+ }
+
+ const data = await response.json().catch(() => ({}));📝 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.
| // Get the response data | |
| const data = await response.json(); | |
| // Get the response data | |
| if (!response.ok) { | |
| const errorText = await response.text().catch(() => 'Unknown error'); | |
| return new Response( | |
| JSON.stringify({ error: `Coinbase API error (${response.status}): ${errorText}` }), | |
| { | |
| status: response.status, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*', | |
| }, | |
| } | |
| ); | |
| } | |
| const data = await response.json().catch(() => ({})); |
| } catch (error) { | ||
| return new Response(JSON.stringify({ error: error.message }), { | ||
| status: 500, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Access-Control-Allow-Origin': '*', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid exposing raw error messages.
Line 43 exposes error.message directly in the response, which could leak sensitive information like internal paths, stack traces, or implementation details if an unexpected error occurs.
Apply this diff:
} catch (error) {
- return new Response(JSON.stringify({ error: error.message }), {
+ return new Response(JSON.stringify({ error: 'Failed to process request' }), {
status: 500,
headers: {
'Content-Type': 'application/json',
'Access-Control-Allow-Origin': '*',
},
});
}📝 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.
| } catch (error) { | |
| return new Response(JSON.stringify({ error: error.message }), { | |
| status: 500, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*', | |
| }, | |
| }); | |
| } | |
| } catch (error) { | |
| return new Response(JSON.stringify({ error: 'Failed to process request' }), { | |
| status: 500, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*', | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In functions/api/coinbase/platform/v2/onramp/sessions.js around lines 42 to 50,
the catch block currently returns error.message to the client which can leak
sensitive implementation details; change the response to return a generic error
message (e.g., { error: "Internal server error" } or a short error code) and
keep the original error for server-side logging only (e.g., console.error or a
logger) so details are recorded but not exposed to the user; ensure the response
status and headers remain the same and the response body stays valid JSON.
| // Get token from backend API | ||
| const tokenUrl = new URL(import.meta.env.VITE_ONRAMP_JWT_URL); | ||
|
|
||
| const tokenResponse = await fetch(tokenUrl.toString(), { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| if (!tokenResponse.ok) { | ||
| throw new Error('Failed to get token from backend'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the environment variable is documented.
The code relies on VITE_ONRAMP_JWT_URL (line 55) but this environment variable may not be documented in the project's configuration guide. Ensure it's added to .env.example or relevant documentation.
Check if the environment variable is documented:
🏁 Script executed:
#!/bin/bash
# Check for .env.example or documentation of VITE_ONRAMP_JWT_URL
fd -t f "\.env\.example|README|SETUP|CONFIG" -X grep -l "VITE_ONRAMP_JWT_URL"Length of output: 0
🏁 Script executed:
#!/bin/bash
# 1. Search for all .env* files
echo "=== .env files ==="
fd -t f "\.env"
# 2. Search for all VITE_ONRAMP_JWT_URL references in codebase
echo -e "\n=== VITE_ONRAMP_JWT_URL references ==="
rg -n "VITE_ONRAMP_JWT_URL" --type=ts --type=tsx --type=js --type=json
# 3. Search for environment variable documentation patterns
echo -e "\n=== Env var docs (*.md files) ==="
fd -t f "\.md$" -x grep -l "environment\|VITE_" 2>/dev/null | head -20
# 4. Check for any env example or config documentation
echo -e "\n=== Config/env related files ==="
fd -t f "\.(example|template|sample)" | head -20Length of output: 197
🏁 Script executed:
#!/bin/bash
# 1. Search for VITE_ONRAMP_JWT_URL with corrected rg syntax
echo "=== VITE_ONRAMP_JWT_URL references ==="
rg -n "VITE_ONRAMP_JWT_URL"
# 2. Find the WalletPortfolioButtons.tsx file
echo -e "\n=== WalletPortfolioButtons.tsx location ==="
fd -t f "WalletPortfolioButtons.tsx"
# 3. Check for vite.config files
echo -e "\n=== Vite config files ==="
fd -t f "vite\.config\.(ts|js|mts|mjs)"
# 4. Search for environment documentation in markdown/docs
echo -e "\n=== All markdown files ==="
fd -t f "\.md$" | head -30
# 5. Check root level for env documentation
echo -e "\n=== Root level files ==="
fd -maxdepth 1 -t fLength of output: 696
🏁 Script executed:
#!/bin/bash
# 1. Check vite.config.js for env var documentation
echo "=== vite.config.js content ==="
cat vite.config.js
# 2. Check README.md for environment documentation
echo -e "\n=== README.md (first 100 lines) ==="
head -100 README.md
# 3. Search for VITE_ONRAMP_JWT_URL in all non-ignored files
echo -e "\n=== All VITE_ONRAMP_JWT_URL occurrences ==="
rg "VITE_ONRAMP_JWT_URL" -A 2 -B 2
# 4. Search for environment variable setup documentation
echo -e "\n=== Environment setup docs ==="
rg "environment|env\s*setup|VITE_" --type=mdLength of output: 3639
🏁 Script executed:
#!/bin/bash
# 1. Check .gitignore for env file patterns
echo "=== .gitignore content ==="
cat .gitignore 2>/dev/null || echo "No .gitignore found"
# 2. Use git to find .env.example (even if ignored)
echo -e "\n=== Git check for .env.example ==="
git ls-files --others --ignored --exclude-standard | grep -E "\.env"
# 3. Directly look for .env.example with any pattern
echo -e "\n=== Direct search for env example files ==="
find . -name "*env*example*" -o -name ".env.example" 2>/dev/null | head -20
# 4. Check git log to see if .env.example was ever committed
echo -e "\n=== Git history for .env files ==="
git log --all --full-history --oneline -- ".env*" 2>/dev/null | head -10Length of output: 1110
🏁 Script executed:
#!/bin/bash
# Check .env.example contents
echo "=== .env.example content ==="
cat .env.example
# Specifically search for VITE_ONRAMP_JWT_URL in .env.example
echo -e "\n=== Search for VITE_ONRAMP_JWT_URL in .env.example ==="
grep -i "VITE_ONRAMP_JWT_URL" .env.example || echo "NOT FOUND in .env.example"
# Show all VITE_ variables documented
echo -e "\n=== All VITE_ variables in .env.example ==="
grep "VITE_" .env.exampleLength of output: 1681
Add VITE_ONRAMP_JWT_URL to .env.example documentation.
The code uses VITE_ONRAMP_JWT_URL (line 55) but this environment variable is missing from .env.example. Add it to the environment configuration template so developers can properly set up their local development environment. Include a description of what the variable should contain (e.g., the URL for the onramp JWT token endpoint).
🤖 Prompt for AI Agents
In
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
around lines 54 to 66, the component reads VITE_ONRAMP_JWT_URL to fetch a JWT
token but that env var is not documented in .env.example; update .env.example to
add VITE_ONRAMP_JWT_URL with a brief description (e.g., "URL for onramp JWT
token endpoint used to fetch onramp auth tokens") and an example value
placeholder (e.g., https://api.example.com/onramp/jwt) so developers know to
configure it for local development.
yeah sure the reason for adding Cloudflare functions is that when Coinbase API was directly called we had CORS issue I tired to change the vote config to use proxy server but it does not work in production does work only on dev mode vitejs/vite#17381 so had to use redirects on Cloudflare in order to access that url on Coinbase Btw I also gave a try on Access-Cross-Control-Origin to * or specifically to our host url it didn't work |
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (1)
147-159: Parse JSON only after validating response status.Line 147 calls
response.json()before checkingresponse.okon line 152. If the Coinbase API returns a non-2xx status with an empty body (e.g., 204 No Content) or invalid JSON, the parse will throw an exception that masks the actual HTTP error. The catch block will then surface a generic parsing error instead of the meaningful API error message.Apply this diff to check status before parsing:
const response = await fetch( '/api/coinbase/platform/v2/onramp/sessions', { method: 'POST', headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json', }, body: JSON.stringify({ purchaseCurrency: 'USDC', destinationNetwork: 'base', destinationAddress: accountAddress, paymentCurrency: 'USD', clientIp, }), } ); - const data = await response.json(); - - // eslint-disable-next-line no-console - console.log('Coinbase API response:', { status: response.status, data }); - if (!response.ok) { + const errorBody = await response.text().catch(() => ''); - const errorMsg = `Coinbase API error (${response.status}): ${data.error || JSON.stringify(data)}`; + const errorMsg = `Coinbase API error (${response.status}): ${errorBody}`; // eslint-disable-next-line no-console console.error(errorMsg); // eslint-disable-next-line no-alert alert(errorMsg); return; } + + const data = await response.json(); + + // eslint-disable-next-line no-console + console.log('Coinbase API response:', { status: response.status, data });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
📚 Learning: 2025-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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.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/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
🧬 Code graph analysis (1)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (3)
src/apps/pillarx-app/hooks/useReducerHooks.tsx (1)
useAppDispatch(5-5)src/hooks/useEIP7702Upgrade.tsx (1)
useEIP7702Upgrade(31-188)src/apps/pillarx-app/reducer/WalletPortfolioSlice.ts (1)
setIsReceiveModalOpen(134-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (3)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (3)
1-29: LGTM: Clean imports and state initialization.The imports are well-organized, loading state is properly initialized, and the rename from
walletAddresstoaccountAddressimproves clarity.
176-186: LGTM: Proper error handling and cleanup.The catch block appropriately logs errors and notifies the user, while the finally block ensures the loading state is always reset, preventing the button from remaining stuck in a loading state.
188-243: LGTM: Well-structured UI with proper loading states.The two-row layout cleanly organizes the actions, and the Add Cash button properly reflects its loading state by disabling the button and displaying a spinner. Conditional rendering of the WalletConnectDropdown and Upgrade button is handled correctly.
| console.log('Opening onramp URL:', onrampUrl); | ||
|
|
||
| // Open the URL directly - simpler approach that works on all browsers | ||
| window.open(onrampUrl, '_top', 'noreferrer'); |
There was a problem hiding this comment.
Reconsider _top target—user loses wallet context.
Opening the onramp URL with window.open(onrampUrl, '_top', 'noreferrer') replaces the entire current page, navigating the user away from the wallet interface. Once the user completes (or abandons) the onramp flow, they must manually navigate back. This creates friction and may confuse users.
Consider using '_blank' to open the onramp in a new tab, allowing users to keep the wallet interface accessible in the original tab.
Apply this diff to open in a new tab:
- window.open(onrampUrl, '_top', 'noreferrer');
+ window.open(onrampUrl, '_blank', 'noopener,noreferrer');📝 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.
| window.open(onrampUrl, '_top', 'noreferrer'); | |
| window.open(onrampUrl, '_blank', 'noopener,noreferrer'); |
🤖 Prompt for AI Agents
In
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
around line 175, change the window.open call so it opens the onramp in a new tab
instead of replacing the current page: replace the target '_top' with '_blank'
and include 'noopener,noreferrer' in the features string (or otherwise set
window.opener = null) to avoid opening-page security/OPENER vulnerabilities
while preserving the wallet UI in the original tab.
| @@ -0,0 +1,11 @@ | |||
| { | |||
There was a problem hiding this comment.
What is this file for excatly?
| // Validate wallet address | ||
| if (!accountAddress) { | ||
| // eslint-disable-next-line no-alert | ||
| alert('Wallet address is not available'); |
There was a problem hiding this comment.
Are we sure we want to provide alert like this? Rather than error message in a UI we create?
There was a problem hiding this comment.
i will change that was debugging something
| const isValidAddress = /^0x[a-fA-F0-9]{40}$/.test(accountAddress); | ||
| if (!isValidAddress) { | ||
| // eslint-disable-next-line no-alert | ||
| alert('Invalid wallet address format'); |
There was a problem hiding this comment.
Same comment on alert here
| } | ||
|
|
||
| // Check if address is a valid Ethereum address (0x followed by 40 hex characters) | ||
| const isValidAddress = /^0x[a-fA-F0-9]{40}$/.test(accountAddress); |
There was a problem hiding this comment.
You can use a helper function from Viem to check if it is an address here
There was a problem hiding this comment.
@vignesha22 for consistency please change this
| tokenResponse.statusText | ||
| ); | ||
| // eslint-disable-next-line no-alert | ||
| alert( |
There was a problem hiding this comment.
Same comment on alert, for this and the next ones
| </button> | ||
| {shouldShowWalletConnectDropdown && <WalletConnectDropdown />} | ||
| </div> | ||
| <div className="flex w-full desktop:gap-x-2.5 tablet:gap-x-2.5"> |
There was a problem hiding this comment.
I think the AddCash button should be a separate component rather than all coded here
| userPortfolio | ||
| ); | ||
| } else { | ||
| // eslint-disable-next-line @typescript-eslint/no-use-before-define |
There was a problem hiding this comment.
Try not ignoring this eslint here
| * Get the best sell offer with cross-chain bridge | ||
| * This calculates the final amount user will receive after: | ||
| * 1. Swapping token to USDC on same chain | ||
| * 2. Taking 1% fee from received USDC |
There was a problem hiding this comment.
This description is incorrect, it should be taking 1% fee of the token that we sell
There was a problem hiding this comment.
yes since we are taking the fee as USDC on the chain they are selling the token and btw the description is right
|
|
||
| // Step 2: Calculate USDC after first swap and fee (1%) | ||
| // User gets 99% of the USDC from the first swap | ||
| const usdcAfterFirstSwap = firstSwapOffer.tokenAmountToReceive; |
There was a problem hiding this comment.
??? we are taking 1% from USDC not from the token we sell???
There was a problem hiding this comment.
yes so now the flow is User swaps the sell token to USDC on same chain and we take 1% of the received amount and then the rest is swapped again to their destination chain
| const clearError = useCallback(() => { | ||
| setError(null); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Honestly, there is so much new logic in place, different way of taking fees etc. It is very confusing and quite hard to review at this stage
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
requested a deployment
to
Preview (feature/PRO-3781-Add_Cash)
github-actions
bot
requested a deployment
to
Preview (feature/PRO-3781-Add_Cash)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
vite.config.js (1)
33-33: The basicSsl plugin remains unused with HTTPS disabled.This contradictory configuration was previously flagged: the
basicSsl()plugin (lines 6, 9) is included buthttps: falsedisables HTTPS, making the plugin serve no purpose.
🧹 Nitpick comments (1)
vite.config.js (1)
35-42: Document that this proxy is for local development only.The proxy configuration is technically correct, but per the PR objectives, this proxy works in development but not in production. The Cloudflare function at
functions/api/coinbase/...handles the production case. Consider adding a comment to clarify this limitation.Apply this diff to add clarifying documentation:
proxy: { + // Dev-only proxy for Coinbase API. Production uses Cloudflare function at /functions/api/coinbase/ '/api/coinbase': { target: 'https://api.cdp.coinbase.com', changeOrigin: true, rewrite: (path) => path.replace(/^\/api\/coinbase/, ''), secure: true, }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(2 hunks)vite.config.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
🔇 Additional comments (1)
vite.config.js (1)
13-15: LGTM! Correct exclusion of Cloudflare functions from the build.Excluding
/functions/**prevents Cloudflare Pages functions from being bundled with the frontend code, which is the correct approach.
github-actions
bot
temporarily deployed
to
Preview (feature/PRO-3781-Add_Cash)
| kit.getEtherspotProvider().getWalletMode() !== 'delegatedEoa'; | ||
|
|
||
| const handleAddCash = async () => { | ||
| if (isAddCashLoading) return; |
There was a problem hiding this comment.
Can we show something to the user here so they're not ghosted - just an alert telling them that it's still loading and to try again shortly
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
| } | ||
|
|
||
| // Check if address is a valid Ethereum address (0x followed by 40 hex characters) | ||
| const isValidAddress = /^0x[a-fA-F0-9]{40}$/.test(accountAddress); |
There was a problem hiding this comment.
@vignesha22 for consistency please change this
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Show resolved
Hide resolved
| {isAddCashLoading ? ( | ||
| <> | ||
| <span className="text-white font-medium text-[14px] leading-[14px] tracking-[-0.02em] text-center align-bottom"> | ||
| Add Cash | ||
| </span> | ||
| <TailSpin color="#FFFFFF" height={16} width={16} /> | ||
| </> | ||
| ) : ( | ||
| <> | ||
| <span className="text-white font-medium text-[14px] leading-[14px] tracking-[-0.02em] text-center align-bottom"> | ||
| Add Cash | ||
| </span> | ||
| <img src={FrameIcon} alt="Add Cash" width={16} height={16} /> | ||
| </> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
This inline if/else is nearly the same - could the confitional value not be just for <TailSpin />?
vite.config.js
Outdated
| }, | ||
| server: { | ||
| https: true, | ||
| https: false, |
There was a problem hiding this comment.
Keep this enabled for other developers - HTTPS should be turned on as standard
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
UI
Chores