Conversation
WalkthroughRemoves PrivateKeyLoginProvider and its hook; replaces provider-based PK flow with in-memory privateKey plus localStorage session flag and Pillar Wallet WebView messaging; propagates privateKey into Authorized/Etherspot config; updates Main auth/provider wiring, logout behavior, tests, and RN UI integration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Main
participant Msg as pillarWalletMessaging
participant RN as ReactNativeWebView
participant Storage as localStorage
participant Auth as Authorized
Note over App,Msg: RN WebView private-key request flow (new)
App->>Msg: setupPillarWalletMessaging(onSuccess,onError)
Msg->>RN: postMessage({type:"pillarXAuthRequest", value:"pk"})
RN-->>Msg: message {type:"pillarWalletPkResponse", value:{pk}}
Msg->>Msg: validate pk -> derive address
Msg-->>App: onSuccess(address, privateKey)
App->>Storage: set `ACCOUNT_VIA_PK` = address
App->>App: setState(pkAccount, privateKey)
App->>Auth: render <Authorized privateKey={privateKey} ...>
Auth->>Auth: include privateKey in kitConfig (provider from privateKey)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
github-actions
bot
requested a deployment
to
Preview (feature/pillar-wallet-login)
Deploying x with
|
| Latest commit: |
edc7b68
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ad887b92.x-e62.pages.dev |
| Branch Preview URL: | https://feature-pillar-wallet-login.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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/containers/Main.tsx (2)
15-15: Remove unused imports.The imports
isAddressandprivateKeyToAccountare no longer used in this file after removing the private-key login flow.Apply this diff:
import { createWalletClient, custom, http, - isAddress, WalletClient, } from 'viem'; -import { privateKeyToAccount } from 'viem/accounts'; import { mainnet, sepolia } from 'viem/chains';Also applies to: 18-18
160-506: Fix formatting across provider setup.Prettier has flagged extensive formatting issues in the provider setup logic (lines 160-506). These formatting issues affect code readability and consistency.
Run Prettier to auto-fix these issues:
#!/bin/bash # Description: Auto-fix formatting issues in Main.tsx prettier --write src/containers/Main.tsx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/BottomMenu/BottomMenu.test.tsx(2 hunks)src/components/BottomMenu/index.tsx(1 hunks)src/components/BottomMenuModal/AccountModal.tsx(0 hunks)src/containers/Authorized.tsx(2 hunks)src/containers/Main.tsx(7 hunks)src/hooks/usePrivateKeyLogin.tsx(0 hunks)src/pages/ViaPillarWallet.tsx(1 hunks)src/pages/__tests__/ViaPillarWallet.test.tsx(1 hunks)src/providers/PrivateKeyLoginProvider.tsx(0 hunks)src/test-utils/testUtils.tsx(1 hunks)src/utils/logout.ts(0 hunks)
💤 Files with no reviewable changes (4)
- src/components/BottomMenuModal/AccountModal.tsx
- src/providers/PrivateKeyLoginProvider.tsx
- src/utils/logout.ts
- src/hooks/usePrivateKeyLogin.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (2)
src/containers/Main.tsx (1)
src/utils/blockchain.ts (1)
isTestnet(33-39)
src/pages/__tests__/ViaPillarWallet.test.tsx (1)
src/theme/index.ts (1)
defaultTheme(83-163)
🪛 GitHub Actions: IAmKio has committed new code! 🚀 Checking...
src/pages/ViaPillarWallet.tsx
[error] 7-7: Missing file extension for "../hooks/usePrivateKeyLogin" import/extensions
[error] 45-45: Prettier formatting issue detected
[error] 204-204: no-console: Unexpected console statement
[error] 321-321: Prettier formatting issue detected
src/containers/Main.tsx
[error] 15-15: 'isAddress' is defined but never used
[error] 18-18: 'privateKeyToAccount' is defined but never used
[error] 78-78: Prettier formatting issue detected
[error] 160-160: Prettier formatting issue detected
[error] 161-161: Prettier formatting issue detected
[error] 162-162: Prettier formatting issue detected
[error] 163-163: Prettier formatting issue detected
[error] 164-164: Prettier formatting issue detected
[error] 165-165: Prettier formatting issue detected
[error] 166-166: Prettier formatting issue detected
[error] 167-167: Prettier formatting issue detected
[error] 168-168: Prettier formatting issue detected
[error] 169-169: Prettier formatting issue detected
[error] 170-170: Prettier formatting issue detected
[error] 171-171: Prettier formatting issue detected
[error] 172-172: Prettier formatting issue detected
[error] 173-173: Prettier formatting issue detected
[error] 175-175: Prettier formatting issue detected
[error] 176-176: Prettier formatting issue detected
[error] 177-177: Prettier formatting issue detected
[error] 178-178: Prettier formatting issue detected
[error] 179-179: Prettier formatting issue detected
[error] 180-180: Prettier formatting issue detected
[error] 181-181: Prettier formatting issue detected
[error] 182-182: Prettier formatting issue detected
[error] 183-183: Prettier formatting issue detected
[error] 184-184: Prettier formatting issue detected
[error] 185-185: Prettier formatting issue detected
[error] 186-186: Prettier formatting issue detected
[error] 187-187: Prettier formatting issue detected
[error] 188-188: Prettier formatting issue detected
[error] 189-189: Prettier formatting issue detected
[error] 190-190: Prettier formatting issue detected
[error] 191-191: Prettier formatting issue detected
[error] 192-192: Prettier formatting issue detected
[error] 193-193: Prettier formatting issue detected
[error] 194-194: Prettier formatting issue detected
[error] 195-195: Prettier formatting issue detected
[error] 197-197: Prettier formatting issue detected
src/pages/__tests__/ViaPillarWallet.test.tsx
[error] 7-7: @sentry/react import should occur before import of ../ViaPillarWallet import/order
[error] 45-45: prettier/prettier: Replace 'screen.getByText('Connecting to Pillar Wallet...')' with proper line breaks
[error] 68-68: prettier/prettier: Insert line breaks
🔇 Additional comments (4)
src/test-utils/testUtils.tsx (1)
124-126: LGTM!The removal of
PrivateKeyLoginProviderwrapper aligns with the broader refactor to eliminate the private-key login flow. The simplified provider tree maintains necessary context throughSelectedChainsHistoryProvider.src/components/BottomMenu/index.tsx (1)
81-81: LGTM!The simplified authentication guard correctly replaces account-based checks with Privy/Wagmi authentication state. This aligns with the removal of the private-key login flow across the codebase.
src/components/BottomMenu/BottomMenu.test.tsx (1)
62-66: LGTM!The test provider tree correctly reflects the removal of
PrivateKeyLoginProviderand maintains proper nesting withBottomMenuModalProviderandSelectedChainsHistoryProvider.Also applies to: 101-105
src/containers/Main.tsx (1)
572-575: LGTM!The
/via-pillar-walletroute is correctly added to both authorized and unauthorized route definitions, and included in theisRootPagecheck. This enables the ViaPillarWallet component to be accessed regardless of authentication state.Also applies to: 696-699
src/containers/Main.tsx
Outdated
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [wallets, user, account, wagmiIsConnected]); | ||
| }, [wallets, user, wagmiIsConnected]); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removal of dependencies from effect.
The dependency array for the provider setup effect was changed, removing several dependencies (likely account, connectors, etc.). Ensure this doesn't cause stale closures where the effect doesn't re-run when it should.
The effect now only depends on [wallets, user, wagmiIsConnected] but references connectors inside the effect (lines 285-299, 438). If connectors changes but the effect doesn't re-run, the component may use stale connector references.
Run the following script to trace all variables referenced inside the effect:
🏁 Script executed:
#!/bin/bash
# Description: Find all variables referenced in the provider setup effect
# Extract the effect body (lines 144-510) and find all identifier references
ast-grep --pattern $'useEffect(() => {
$$$
}, [$DEPS])' | rg -n '\b(connectors|ready|authenticated)\b'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Show useEffect occurrences with surrounding context in Main.tsx
echo "UseEffect occurrences:"
rg -n -C3 'useEffect\(\s*\(\s*\)\s*=>' --type=tsx src/containers/Main.tsx
# Search for all 'connectors' references in Main.tsx
echo "All 'connectors' references:"
rg -n 'connectors' src/containers/Main.tsx
# Extract lines 144-510 and search for 'connectors' within that block
echo "Search for 'connectors' in lines 144-510:"
sed -n '144,510p' src/containers/Main.tsx | grep -n 'connectors'Length of output: 1050
Include connectors in the provider setup effect’s dependency array
The effect calls connectors.find(...) and connectors.map(...) (e.g. around lines 285–299 and 438) but only lists [wallets, user, wagmiIsConnected]. Add connectors to the array to prevent stale references.
🤖 Prompt for AI Agents
In src/containers/Main.tsx around line 510, the effect currently lists [wallets,
user, wagmiIsConnected] but uses the connectors variable inside (calls like
connectors.find and connectors.map), which can lead to stale references; add
connectors to the dependency array so the effect re-runs when connectors change,
i.e., update the array to include connectors alongside the existing
dependencies.
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | ||
| import ViaPillarWallet from '../ViaPillarWallet'; | ||
| import { defaultTheme } from '../../theme'; | ||
| import * as Sentry from '@sentry/react'; |
There was a problem hiding this comment.
Fix import order.
The @sentry/react import should come before the relative import ../ViaPillarWallet to comply with the import/order rule.
Apply this diff:
+import * as Sentry from '@sentry/react';
import { render, screen, waitFor } from '@testing-library/react';
import { MemoryRouter } from 'react-router-dom';
import { ThemeProvider } from 'styled-components';
import { describe, it, expect, beforeEach, vi } from 'vitest';
import ViaPillarWallet from '../ViaPillarWallet';
import { defaultTheme } from '../../theme';
-import * as Sentry from '@sentry/react';📝 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.
| import * as Sentry from '@sentry/react'; | |
| import * as Sentry from '@sentry/react'; | |
| import { render, screen, waitFor } from '@testing-library/react'; | |
| import { MemoryRouter } from 'react-router-dom'; | |
| import { ThemeProvider } from 'styled-components'; | |
| import { describe, it, expect, beforeEach, vi } from 'vitest'; | |
| import ViaPillarWallet from '../ViaPillarWallet'; | |
| import { defaultTheme } from '../../theme'; |
🧰 Tools
🪛 GitHub Actions: IAmKio has committed new code! 🚀 Checking...
[error] 7-7: @sentry/react import should occur before import of ../ViaPillarWallet import/order
🤖 Prompt for AI Agents
In src/pages/__tests__/ViaPillarWallet.test.tsx around line 7, the import from
'@sentry/react' is placed after the relative import '../ViaPillarWallet';
reorder the imports so that the external module import (@sentry/react) appears
before the relative import to satisfy the import/order rule — simply move the
`import * as Sentry from '@sentry/react';` line above the `import` of
`../ViaPillarWallet`.
| vi.mock('../../hooks/usePrivateKeyLogin', () => ({ | ||
| default: () => ({ | ||
| setAccount: vi.fn(), | ||
| account: null, | ||
| }), | ||
| })); |
There was a problem hiding this comment.
Critical: Mocking a deleted hook.
The test mocks usePrivateKeyLogin from '../../hooks/usePrivateKeyLogin', but according to the PR summary, this hook was removed (see src/hooks/usePrivateKeyLogin.tsx in the AI-generated summary). The actual ViaPillarWallet.tsx component imports this deleted hook on line 7, which will cause a runtime error.
This indicates a fundamental issue: the ViaPillarWallet component relies on a hook that no longer exists. You need to either:
- Restore the
usePrivateKeyLoginhook, or - Refactor
ViaPillarWallet.tsxto use an alternative authentication mechanism that aligns with the new Privy/Wagmi-only flow
🤖 Prompt for AI Agents
In src/pages/__tests__/ViaPillarWallet.test.tsx around lines 11 to 16, the test
is mocking usePrivateKeyLogin which no longer exists and will cause runtime
failures; either restore the deleted hook or refactor the component and tests to
use the new authentication flow—update ViaPillarWallet.tsx to remove the import
of usePrivateKeyLogin and replace its usage with the new Privy/Wagmi-based auth
API (or a small adapter hook that exposes setAccount/account via the new flow),
then update this test to mock the new hook or adapter (or remove the mock if the
component no longer needs it) and adjust imports to the correct path so tests
run without referencing the deleted file.
| it('renders loading state initially', () => { | ||
| renderComponent(); | ||
| expect(screen.getByText('Connecting to Pillar Wallet...')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Fix formatting for readability.
Prettier flagged line 45 for improper line breaks. Consider breaking the assertion into multiple lines for better readability.
Apply this diff:
- expect(screen.getByText('Connecting to Pillar Wallet...')).toBeInTheDocument();
+ expect(
+ screen.getByText('Connecting to Pillar Wallet...')
+ ).toBeInTheDocument();📝 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.
| it('renders loading state initially', () => { | |
| renderComponent(); | |
| expect(screen.getByText('Connecting to Pillar Wallet...')).toBeInTheDocument(); | |
| }); | |
| it('renders loading state initially', () => { | |
| renderComponent(); | |
| expect( | |
| screen.getByText('Connecting to Pillar Wallet...') | |
| ).toBeInTheDocument(); | |
| }); |
🧰 Tools
🪛 GitHub Actions: IAmKio has committed new code! 🚀 Checking...
[error] 45-45: prettier/prettier: Replace 'screen.getByText('Connecting to Pillar Wallet...')' with proper line breaks
🤖 Prompt for AI Agents
In src/pages/__tests__/ViaPillarWallet.test.tsx around lines 43 to 46, Prettier
flagged the assertion line for formatting; split the single long assertion into
two clearer lines for readability — first call screen.getByText(...) and store
the result in a const (e.g., const el = screen.getByText('Connecting to Pillar
Wallet...')), then on the next line assert expect(el).toBeInTheDocument(); this
fixes the line-break/formatting issue while preserving the test behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/containers/Main.tsx (1)
678-679: Authorized now receivesprivateKeyProp is passed; resolves the earlier missing-prop issue flagged previously.
🧹 Nitpick comments (5)
src/components/BottomMenu/index.tsx (1)
42-43: Avoid scattering auth state reads across componentsReading localStorage directly couples UI to storage keys. Prefer a single auth store/hook (e.g., useAuth) that exposes isPkAccount and emits changes (storage/focus). Keeps concerns centralized and avoids drift.
src/apps/pillarx-app/index.tsx (2)
191-206: Unify message schema and tighten postMessage usage
- You send { type: 'pillarXAuthRequest', value: 'settings' } here, while the messaging module defines the same type for value 'pk'. Centralize message types/values in a shared constants module to avoid drift.
- For the browser fallback, avoid '*' when possible, or route all messaging through a shared helper (e.g., postToRNWebView) that encapsulates targets and logging.
43-45: RN detection via localStorage is staticSince this const is read once, UI won’t react if DEVICE_PLATFORM changes at runtime. If needed, lift into state with a storage/focus listener (similar to your debug flag) for reactivity.
src/utils/pillarWalletMessaging.ts (1)
194-232: Minor: request timing and cleanup are fine; consider passing requestId to RN in docsEnsure the RN side echoes requestId back in pillarWalletPkResponse. If backward-compat is required, keep the handler tolerant (as above).
src/containers/Main.tsx (1)
646-648: Addconnectorsto effect depsThe effect body uses connectors (find/map). Add it to the dependency array to avoid stale references.
Apply:
- }, [wallets, user, wagmiIsConnected, privateKey, pkAccount]); + }, [wallets, user, wagmiIsConnected, privateKey, pkAccount, connectors]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pillarx-app/index.tsx(4 hunks)src/components/BottomMenu/index.tsx(2 hunks)src/components/BottomMenuModal/AccountModal.tsx(3 hunks)src/containers/Authorized.tsx(2 hunks)src/containers/Main.tsx(9 hunks)src/utils/pillarWalletMessaging.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/BottomMenuModal/AccountModal.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (2)
src/apps/pillarx-app/index.tsx (1)
src/apps/pillarx-app/components/PillarXLogo/PillarXLogo.tsx (1)
PillarXLogo(17-105)
src/containers/Main.tsx (4)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(194-232)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/utils/blockchain.ts (2)
visibleChains(161-163)isTestnet(33-39)src/containers/Authorized.tsx (1)
Authorized(30-162)
⏰ 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 (5)
src/components/BottomMenu/index.tsx (1)
84-86: Guard logic looks correctMenu hides unless authenticated via any method (Privy, wagmi, or PK). Good integration with new PK path.
src/apps/pillarx-app/index.tsx (1)
210-221: Header/settings integration LGTMConditional RN settings button, accessible label, and centered logo are implemented cleanly.
src/containers/Authorized.tsx (2)
33-38: Prop surface extension looks goodAdding optional privateKey to Authorized matches the new auth path and maintains backward compatibility.
119-123: Correct to include privateKey in kitConfig and depsGood memoization; config updates when PK changes.
src/containers/Main.tsx (1)
254-297: PK provider setup path looks soundPriority selection and chainId fallback to visibleChains[0] are reasonable for initial WIP.
Confirm EtherspotTransactionKitProvider accepts a local account client created via privateKeyToAccount and that chainId mismatches are handled by your batches logic as commented.
src/containers/Main.tsx
Outdated
| // Check if there's a stored private key on mount (for session restoration) | ||
| const storedPk = localStorage.getItem('PK_VIA_PK'); | ||
| const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK'); | ||
| if (storedPk && storedAccount) { | ||
| Sentry.addBreadcrumb({ | ||
| category: 'authentication', | ||
| message: 'Restoring private key from localStorage', | ||
| level: 'info', | ||
| data: { | ||
| accountAddress: storedAccount, | ||
| }, | ||
| }); | ||
|
|
||
| setPrivateKey(storedPk); | ||
| setPkAccount(storedAccount); | ||
| } |
There was a problem hiding this comment.
Avoid restoring PK from localStorage by default
Only attempt restoration if explicitly allowed (e.g., in non-production/dev). Better: always reacquire from RN on app start.
Apply:
- const storedPk = localStorage.getItem('PK_VIA_PK');
- const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK');
- if (storedPk && storedAccount) {
+ const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK');
+ const storedPk =
+ import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true'
+ ? localStorage.getItem('PK_VIA_PK')
+ : null;
+ if (storedPk && storedAccount) {
Sentry.addBreadcrumb({
category: 'authentication',
message: 'Restoring private key from localStorage',📝 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.
| // Check if there's a stored private key on mount (for session restoration) | |
| const storedPk = localStorage.getItem('PK_VIA_PK'); | |
| const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK'); | |
| if (storedPk && storedAccount) { | |
| Sentry.addBreadcrumb({ | |
| category: 'authentication', | |
| message: 'Restoring private key from localStorage', | |
| level: 'info', | |
| data: { | |
| accountAddress: storedAccount, | |
| }, | |
| }); | |
| setPrivateKey(storedPk); | |
| setPkAccount(storedAccount); | |
| } | |
| // Check if there's a stored private key on mount (for session restoration) | |
| const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK'); | |
| const storedPk = | |
| import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true' | |
| ? localStorage.getItem('PK_VIA_PK') | |
| : null; | |
| if (storedPk && storedAccount) { | |
| Sentry.addBreadcrumb({ | |
| category: 'authentication', | |
| message: 'Restoring private key from localStorage', | |
| level: 'info', | |
| data: { | |
| accountAddress: storedAccount, | |
| }, | |
| }); | |
| setPrivateKey(storedPk); | |
| setPkAccount(storedAccount); | |
| } |
🤖 Prompt for AI Agents
In src/containers/Main.tsx around lines 209-224, the code automatically restores
a private key and account from localStorage on mount; change this so restoration
only happens when explicitly allowed: add a guard (e.g., environment check or
feature flag like ALLOW_PK_RESTORE=true or NODE_ENV !== 'production') around the
localStorage read and Sentry breadcrumb, otherwise skip restoration and trigger
the RN re-acquisition flow on app start; remove unconditional
setPrivateKey/setPkAccount and ensure stored PK is not used in production
(optionally clear it or log a warning when skipped).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/BottomMenuModal/AccountModal.tsx(4 hunks)src/pages/Landing.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Landing.jsx (1)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(194-232)
⏰ 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
| const cleanup = setupPillarWalletMessaging( | ||
| (address, pk) => { | ||
| // Success - store and redirect | ||
| localStorage.setItem('ACCOUNT_VIA_PK', address); | ||
| localStorage.setItem('PK_VIA_PK', pk); | ||
| navigate('/'); | ||
| }, | ||
| (error) => { | ||
| // Error - still redirect but without auth | ||
| console.error('Failed to get private key:', error); | ||
| navigate('/'); | ||
| } | ||
| ); | ||
|
|
||
| return cleanup; | ||
| } | ||
| }, [navigate]); | ||
|
|
||
| // Show loading state if authenticating via React Native | ||
| if (isLoadingAuth) { | ||
| return ( | ||
| <> | ||
| <GlobalStyles /> | ||
| <LoadingContainer> | ||
| <Spinner /> | ||
| <LoadingText>Just a moment</LoadingText> | ||
| </LoadingContainer> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid permanent loading spinner after RN auth flow
The RN auth branch sets isLoadingAuth to true but never clears it. Both success and error callbacks immediately call navigate('/'), and because we remain on the same component, the state stays true, so users get stuck on the loader forever. Reset the flag in both callbacks (and ideally before early navigation) so we return to normal rendering once the messaging flow completes.
const cleanup = setupPillarWalletMessaging(
(address, pk) => {
// Success - store and redirect
localStorage.setItem('ACCOUNT_VIA_PK', address);
localStorage.setItem('PK_VIA_PK', pk);
+ setIsLoadingAuth(false);
navigate('/');
},
(error) => {
// Error - still redirect but without auth
console.error('Failed to get private key:', error);
+ setIsLoadingAuth(false);
navigate('/');
}
);📝 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 cleanup = setupPillarWalletMessaging( | |
| (address, pk) => { | |
| // Success - store and redirect | |
| localStorage.setItem('ACCOUNT_VIA_PK', address); | |
| localStorage.setItem('PK_VIA_PK', pk); | |
| navigate('/'); | |
| }, | |
| (error) => { | |
| // Error - still redirect but without auth | |
| console.error('Failed to get private key:', error); | |
| navigate('/'); | |
| } | |
| ); | |
| return cleanup; | |
| } | |
| }, [navigate]); | |
| // Show loading state if authenticating via React Native | |
| if (isLoadingAuth) { | |
| return ( | |
| <> | |
| <GlobalStyles /> | |
| <LoadingContainer> | |
| <Spinner /> | |
| <LoadingText>Just a moment</LoadingText> | |
| </LoadingContainer> | |
| </> | |
| ); | |
| } | |
| const cleanup = setupPillarWalletMessaging( | |
| (address, pk) => { | |
| // Success - store and redirect | |
| localStorage.setItem('ACCOUNT_VIA_PK', address); | |
| localStorage.setItem('PK_VIA_PK', pk); | |
| setIsLoadingAuth(false); | |
| navigate('/'); | |
| }, | |
| (error) => { | |
| // Error - still redirect but without auth | |
| console.error('Failed to get private key:', error); | |
| setIsLoadingAuth(false); | |
| navigate('/'); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/pages/Landing.jsx around lines 37 to 66, the RN auth flow sets
isLoadingAuth to true but never clears it, causing the permanent loading
spinner; update both the success and error callbacks passed to
setupPillarWalletMessaging to call the state setter (e.g.,
setIsLoadingAuth(false)) before navigating, ensuring the loading flag is reset
on both success and failure so the component can render normally after the
messaging flow completes.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/utils/pillarWalletMessaging.ts (3)
8-13: Add requestId to message types for correlationInclude a requestId to pair requests/responses and ignore unsolicited messages. Make it required on requests, optional on responses for backward-compat.
export type PillarWalletPkResponse = { type: 'pillarWalletPkResponse'; + requestId?: string; value: { pk: string; }; }; export type PillarWalletAuthRequest = { type: 'pillarXAuthRequest'; value: 'pk'; + requestId: string; };Also applies to: 18-21
51-74: Send requestId with the auth requestauthRequestId is not included in the posted message; responses can’t be correlated.
export const requestPrivateKey = (authRequestId: string): void => { const message: PillarWalletAuthRequest = { type: 'pillarXAuthRequest', value: 'pk', + requestId: authRequestId, }; Sentry.addBreadcrumb({ @@ - message: JSON.stringify(message), + message: JSON.stringify(message), }, }); // Send message to React Native webview if (window.ReactNativeWebView) { window.ReactNativeWebView.postMessage(JSON.stringify(message)); } else { // Fallback for testing in browser window.postMessage(JSON.stringify(message), '*'); } };
83-189: Harden handler: validate requestId and stop listening after success
- Parse strings or objects.
- Ignore messages with mismatched requestId.
- Remove listener after success to reduce attack surface.
export const createWebViewMessageHandler = ( authRequestId: string, onSuccess: OnPrivateKeyReceivedCallback, onError: OnErrorCallback ) => { - return (event: MessageEvent) => { + const handler = (event: MessageEvent) => { try { - const data = JSON.parse(event.data); + const data = + typeof (event as any).data === 'string' + ? JSON.parse((event as any).data) + : (event as any).data; @@ data: { authRequestId, messageType: data?.type, hasValue: !!data?.value, + requestId: data?.requestId, }, }); + // Correlate only to this request (allow missing requestId for backward-compat if needed) + if (data?.requestId && data.requestId !== authRequestId) { + return; + } + // Check if this is the private key response if (data?.type === 'pillarWalletPkResponse' && data?.value?.pk) { const privateKey = data.value.pk; @@ // Execute success callback onSuccess(account.address, privateKey); + // Stop listening after success + window.removeEventListener('message', handler); } else { throw new Error('Invalid address derived from private key'); } } catch (error) { @@ } } catch (error) { @@ } - }; + }; + return handler; };src/containers/Main.tsx (3)
172-175: Do not persist plaintext private keysRemove or strictly gate PK persistence. Plaintext PK in localStorage is a critical risk (XSS → instant key theft).
- localStorage.setItem('PK_VIA_PK', pk); + if (import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true') { + localStorage.setItem('PK_VIA_PK', pk); + }
203-218: Avoid restoring PK from localStorage by defaultOnly restore when explicitly allowed (dev/flag). Otherwise reacquire from RN each session.
- const storedPk = localStorage.getItem('PK_VIA_PK'); - const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK'); + const storedAccount = localStorage.getItem('ACCOUNT_VIA_PK'); + const storedPk = + import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true' + ? localStorage.getItem('PK_VIA_PK') + : null; if (storedPk && storedAccount) {
639-641: Addconnectorsto the provider setup effect depsThe effect uses
connectorsbut doesn’t depend on it; may hold stale refs.- }, [wallets, user, wagmiIsConnected, privateKey, pkAccount]); + }, [wallets, user, wagmiIsConnected, privateKey, pkAccount, connectors]);
🧹 Nitpick comments (4)
src/components/BottomMenu/index.tsx (1)
42-44: Make isPkAccount reactive (listen to changes)Reading localStorage once won’t reflect updates; track it in state and subscribe to storage/focus like you do for debug mode.
- const isPkAccount = !!localStorage.getItem('ACCOUNT_VIA_PK'); + const [isPkAccount, setIsPkAccount] = useState( + !!localStorage.getItem('ACCOUNT_VIA_PK') + ); + useEffect(() => { + const checkPk = () => + setIsPkAccount(!!localStorage.getItem('ACCOUNT_VIA_PK')); + checkPk(); + window.addEventListener('storage', checkPk); + window.addEventListener('focus', checkPk); + return () => { + window.removeEventListener('storage', checkPk); + window.removeEventListener('focus', checkPk); + }; + }, []);src/apps/pillarx-app/index.tsx (1)
191-205: Include requestId and reuse messaging util for settings messageAdd a requestId for traceability/correlation and consider centralizing message sending to avoid divergence from auth flow.
- const handleSettingsClick = () => { - const message = JSON.stringify({ - type: 'pillarXAuthRequest', - value: 'settings', - }); + const handleSettingsClick = () => { + const requestId = `settings_${Date.now()}_${Math.random() + .toString(36) + .slice(2, 9)}`; + const message = JSON.stringify({ + type: 'pillarXAuthRequest', + value: 'settings', + requestId, + }); // Send message to React Native webview if (window.ReactNativeWebView) { window.ReactNativeWebView.postMessage(message); } else { // Fallback for testing in browser window.postMessage(message, '*'); } };src/containers/Main.tsx (2)
262-276: Private-key path: pick chain based on env instead of hardcoding 1Defaulting to mainnet may mismatch testnet environments; select from isTestnet/visibleChains.
- const walletChainId = 1; // default chain id is 1 (mainnet) + const walletChainId = isTestnet ? 11155111 /* sepolia */ : 1;Or derive from visibleChains[0].id if you want to ensure support:
- const walletChainId = 1; + const walletChainId = visibleChains[0].id;
622-635: WalletConnect: prefer actual connected chain if availableIf wcProvider exposes chainId, use it instead of forcing 1.
- // For WalletConnect, default to mainnet if no chain ID is set - setChainId(1); + // For WalletConnect, prefer provider's chain, else default to mainnet + const wcChainId = Number(wcProvider?.chainId) || 1; + setChainId(wcChainId);And when creating the client:
- const newProvider = createWalletClient({ + const wcChainId = Number(wcProvider?.chainId) || 1; + const newProvider = createWalletClient({ account: wcAccount as `0x${string}`, - chain: getNetworkViem(1), // Default to mainnet + chain: getNetworkViem(wcChainId), transport: custom(wcProvider), });Also applies to: 451-520
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pillarx-app/index.tsx(4 hunks)src/components/BottomMenu/index.tsx(2 hunks)src/components/BottomMenuModal/AccountModal.tsx(4 hunks)src/containers/Main.tsx(10 hunks)src/pages/Landing.jsx(2 hunks)src/utils/pillarWalletMessaging.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/BottomMenuModal/AccountModal.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (3)
src/pages/Landing.jsx (1)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(197-239)
src/containers/Main.tsx (4)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(197-239)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/utils/blockchain.ts (2)
visibleChains(161-163)isTestnet(33-39)src/containers/Authorized.tsx (1)
Authorized(30-162)
src/apps/pillarx-app/index.tsx (1)
src/apps/pillarx-app/components/PillarXLogo/PillarXLogo.tsx (1)
PillarXLogo(17-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (3)
src/utils/pillarWalletMessaging.ts (1)
197-239: Lifecycle/setup looks solidAuthId generation, listener setup, timer request, and cleanup are correct.
src/components/BottomMenu/index.tsx (1)
84-86: Auth gating logic LGTMThe menu renders if any of authenticated/isConnected/isPkAccount is true.
src/apps/pillarx-app/index.tsx (1)
210-224: Header and Settings UI look goodConditional RN-only Settings button and layout/styling are clear and unobtrusive.
Also applies to: 267-301
| const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2 | ||
|
|
||
| privyEthereumProvider = await walletProvider.getEthereumProvider(); | ||
| const newProvider = createWalletClient({ | ||
| account: walletProvider.address as `0x${string}`, | ||
| chain: getNetworkViem(walletChainId), | ||
| transport: custom(privyEthereumProvider), | ||
| }); | ||
|
|
There was a problem hiding this comment.
Use the matched wallet’s chainId instead of wallets[0]
You locate walletProvider by address, but read chainId from wallets[0]. This can pick the wrong chain if the matched wallet isn’t index 0.
- const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2
+ const walletChainId = +walletProvider.chainId.split(':')[1]; // extract from CAIP-2
@@
- if (wallets.length > 0) {
- const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2
+ if (wallets.length > 0) {
+ const activeWallet = wallets.find(w => w.address === user?.wallet?.address) ?? wallets[0];
+ const walletChainId = +activeWallet.chainId.split(':')[1]; // extract from CAIP-2Also applies to: 597-621
🤖 Prompt for AI Agents
In src/containers/Main.tsx around lines 382 to 389 (and similarly at 597-621),
the code extracts chainId from wallets[0] even though walletProvider was found
by address, which can use the wrong wallet; change the chainId source to the
matched wallet (walletProvider or the variable that references the found wallet)
so you parse chainId from the same wallet object you located by address, then
derive walletChainId from that matched wallet and pass it into getNetworkViem
when creating the new provider.
| // Set up messaging to request private key | ||
| const cleanup = setupPillarWalletMessaging( | ||
| (address, pk) => { | ||
| // Success - store and redirect | ||
| localStorage.setItem('ACCOUNT_VIA_PK', address); | ||
| localStorage.setItem('PK_VIA_PK', pk); | ||
| navigate('/'); | ||
| }, | ||
| (error) => { | ||
| // Error - still redirect but without auth | ||
| console.error('Failed to get private key:', error); | ||
| navigate('/'); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Fix loader reset and avoid storing plaintext PK
- Reset isLoadingAuth before navigating to avoid a permanent spinner if the component remains mounted.
- Never persist raw private keys in localStorage. Keep in memory or gate behind an explicit dev-only flag; prefer secure storage on RN.
const cleanup = setupPillarWalletMessaging(
(address, pk) => {
// Success - store and redirect
localStorage.setItem('ACCOUNT_VIA_PK', address);
- localStorage.setItem('PK_VIA_PK', pk);
+ if (import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true') {
+ localStorage.setItem('PK_VIA_PK', pk);
+ }
+ setIsLoadingAuth(false);
navigate('/');
},
(error) => {
// Error - still redirect but without auth
console.error('Failed to get private key:', error);
+ setIsLoadingAuth(false);
navigate('/');
}
);📝 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.
| // Set up messaging to request private key | |
| const cleanup = setupPillarWalletMessaging( | |
| (address, pk) => { | |
| // Success - store and redirect | |
| localStorage.setItem('ACCOUNT_VIA_PK', address); | |
| localStorage.setItem('PK_VIA_PK', pk); | |
| navigate('/'); | |
| }, | |
| (error) => { | |
| // Error - still redirect but without auth | |
| console.error('Failed to get private key:', error); | |
| navigate('/'); | |
| } | |
| ); | |
| // Set up messaging to request private key | |
| const cleanup = setupPillarWalletMessaging( | |
| (address, pk) => { | |
| // Success - store and redirect | |
| localStorage.setItem('ACCOUNT_VIA_PK', address); | |
| if (import.meta.env.VITE_ALLOW_PLAINTEXT_PK_STORAGE === 'true') { | |
| localStorage.setItem('PK_VIA_PK', pk); | |
| } | |
| setIsLoadingAuth(false); | |
| navigate('/'); | |
| }, | |
| (error) => { | |
| // Error - still redirect but without auth | |
| console.error('Failed to get private key:', error); | |
| setIsLoadingAuth(false); | |
| navigate('/'); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/pages/Landing.jsx around lines 75 to 88, the handler that receives the
private key currently stores the raw PK to localStorage and navigates away
without resetting the loading state; update both the success and error callbacks
to set isLoadingAuth (or equivalent loading state) back to false before calling
navigate('/') so the spinner can't get stuck, remove the
localStorage.setItem('PK_VIA_PK', pk) call (only persist the address if needed),
and instead keep the private key in-memory (React state, context, or a transient
module) or gate persistence behind an explicit dev-only flag; if you must
persist on mobile, use a secure storage mechanism appropriate for React Native
rather than localStorage.
| export type PillarWalletPkResponse = { | ||
| type: 'pillarWalletPkResponse'; | ||
| value: { | ||
| pk: string; | ||
| }; | ||
| }; | ||
|
|
||
| /** | ||
| * Type for the authentication request to the Pillar Wallet webview | ||
| */ | ||
| export type PillarWalletAuthRequest = { | ||
| type: 'pillarXAuthRequest'; | ||
| value: 'pk'; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Update callers/listeners after adding requestId
Search and update all senders/handlers of pillarXAuthRequest/pillarWalletPkResponse to include and check requestId.
Also applies to: 51-74, 83-189
🏁 Script executed:
#!/bin/bash
# Find all message sends and handlers to update for requestId propagation/validation
rg -n -C2 -e "pillarXAuthRequest" -e "pillarWalletPkResponse" -e "postMessage\\(" -e "addEventListener\\(['\"]message" --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Locate all message sends and handlers to update for requestId propagation/validation
rg -n -C2 -e "pillarXAuthRequest" -e "pillarWalletPkResponse" -e "postMessage\(" -e "addEventListener\(['\"]message" -g "*.ts" -g "*.tsx"Length of output: 3077
Propagate requestId across the messaging API
After adding a requestId field to PillarWalletAuthRequest and PillarWalletPkResponse, update all postMessage senders (e.g. in requestPrivateKey and handleSettingsClick) and the messageHandler listener in pillarWalletMessaging.ts to include and validate requestId in the payload.
🤖 Prompt for AI Agents
In src/utils/pillarWalletMessaging.ts around lines 8 to 21, the new requestId
field added to PillarWalletAuthRequest and PillarWalletPkResponse must be
propagated and validated across the messaging API: update both type definitions
to include requestId: string, update all places that call window.postMessage
(notably requestPrivateKey and handleSettingsClick) to attach the same requestId
in the payload, and update the messageHandler listener to validate incoming
messages include a requestId and match it to any outstanding request (reject or
ignore messages with missing/mismatched requestId). Ensure requestId is
preserved verbatim between request and response, and add defensive
checks/logging for missing or invalid requestId before processing the payload.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/pages/Landing.jsx (1)
68-87: ResetisLoadingAuthbefore navigating
setIsLoadingAuth(true)is triggered for the RN flow but the success and error callbacks never revert it. If the navigation keeps this component mounted (e.g., we navigate to '/' from '/'), the state staystrue, leaving users stuck on the loading overlay. CallsetIsLoadingAuth(false)in both callbacks beforenavigate('/').src/containers/Main.tsx (4)
135-141: RN detection should also check localStorage for DEVICE_PLATFORM.The code only reads
devicePlatformfrom URL params, which fails on reloads. Although you store it at line 155, you don't fall back to reading it when the URL param is missing.Apply this diff to check localStorage as a fallback:
const searchParams = new URLSearchParams(window.location.search); const devicePlatform = searchParams.get('devicePlatform'); + const storedPlatform = localStorage.getItem('DEVICE_PLATFORM'); + const resolvedPlatform = devicePlatform || storedPlatform; const isReactNativeApp = - devicePlatform === 'ios' || devicePlatform === 'android'; + resolvedPlatform === 'ios' || resolvedPlatform === 'android';
384-384: Use the matched wallet's chainId instead of wallets[0].You locate
walletProviderby address but extractchainIdfromwallets[0]. If the matched wallet isn't at index 0, you'll use the wrong chain.Apply this diff:
- const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2 + const walletChainId = +walletProvider.chainId.split(':')[1]; // extract from CAIP-2
600-637: Use the correct wallet's chainId (same issue as line 384).At line 601, you again extract
chainIdfromwallets[0]even though the active wallet may not be at index 0. This duplicates the issue at line 384.Apply this diff:
if (wallets.length > 0) { - const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2 + const activeWallet = wallets.find(w => w.address === user?.wallet?.address) ?? wallets[0]; + const walletChainId = +activeWallet.chainId.split(':')[1]; // extract from CAIP-2 const isWithinVisibleChains = visibleChains.some( (chain) => chain.id === walletChainId );
642-642: Includeconnectorsin the dependency array.The effect uses
connectorsat lines 420-433 (finding WalletConnect connector) but doesn't list it in the dependency array. Addconnectorsto prevent stale references.Apply this diff:
// eslint-disable-next-line react-hooks/exhaustive-deps - }, [wallets, user, wagmiIsConnected, privateKey, pkAccount]); + }, [wallets, user, wagmiIsConnected, privateKey, pkAccount, connectors]);
🧹 Nitpick comments (2)
src/containers/Main.tsx (2)
265-265: Consider using stored chain preference instead of hardcoded mainnet.The default
walletChainId = 1works, but you could enhance UX by reading the user's last-used chain from localStorage or inferring it from the stored account's previous session.Example:
- const walletChainId = 1; // default chain id is 1 (mainnet) + const storedChainId = localStorage.getItem('LAST_CHAIN_ID'); + const walletChainId = storedChainId ? parseInt(storedChainId, 10) : 1;
251-638: Consider extracting provider setup into helper functions.The
updateProviderfunction spans ~400 lines handling three authentication flows. While functional, extracting helpers likesetupPrivateKeyProvider,setupPrivyProvider, andsetupWalletConnectProviderwould improve readability and testability.Example structure:
const setupPrivateKeyProvider = async (privateKey: string, pkAccount: string) => { // Lines 253-295 }; const setupPrivyProvider = async (walletProvider: Wallet) => { // Lines 369-404 }; const setupWalletConnectProvider = async () => { // Lines 405-597 }; const updateProvider = async () => { if (privateKey && pkAccount) { return setupPrivateKeyProvider(privateKey, pkAccount); } // ... etc };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/containers/Main.tsx(10 hunks)src/pages/Landing.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (2)
src/containers/Main.tsx (4)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(197-239)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/utils/blockchain.ts (2)
visibleChains(161-163)isTestnet(33-39)src/containers/Authorized.tsx (1)
Authorized(30-162)
src/pages/Landing.jsx (1)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(197-239)
⏰ 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 (2)
src/containers/Main.tsx (2)
166-175: Good security improvements - private key kept in memory only.This correctly addresses the previous security concern by storing only the account address in localStorage (line 174) while keeping the private key in component state. The in-memory approach prevents XSS-based credential theft from at-rest storage.
251-295: Private key provider setup looks correct.The priority logic correctly handles private-key-based authentication by creating a wallet client from the in-memory private key. The flow properly validates both
privateKeyandpkAccountbefore proceeding.
| <LogoutIcon size={20} /> | ||
| </TopBarIcon> | ||
| </Tooltip> | ||
| {!isReactNativeApp && ( |
There was a problem hiding this comment.
How do they logout in react native app then?
There was a problem hiding this comment.
On the home screen there is a new Settings cog icon in the top left to invoke the React Native settings screen where they can log out from there
| const previouslyAuthenticated = | ||
| !!localStorage.getItem('privy:token') || | ||
| localStorage.getItem('ACCOUNT_VIA_PK'); | ||
| const previouslyAuthenticated = !!localStorage.getItem('privy:token'); |
There was a problem hiding this comment.
Do we need to check if previously authenticated with PK?
There was a problem hiding this comment.
With previouslyAuthenticated not being set this loads the unauthorised routes, however for the login via PK method, we always want to start from "zero" i.e. stateless and build up from there. I'm not keen for the app to know that it was in private key mode for security reasons
There was a problem hiding this comment.
ok that's what I thought!
src/containers/Main.tsx
Outdated
| securityNote: 'Private key must be re-requested from RN app', | ||
| }, | ||
| }); | ||
| // Private key will be re-requested when app opens in RN webview again |
There was a problem hiding this comment.
I guess this is a TO DO work
There was a problem hiding this comment.
Sorry this was follow on from the explanation above but is out of place there. WIll remove
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/containers/Main.tsx (3)
362-403: Use matched wallet's chainId instead of wallets[0].Line 383 extracts
chainIdfromwallets[0]even thoughwalletProviderwas found by matchinguser?.wallet?.address. If the matched wallet isn't at index 0, this uses the wrong chain.Apply this diff:
- const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2 + const walletChainId = +walletProvider.chainId.split(':')[1]; // extract from CAIP-2
597-636: Use matched wallet's chainId instead of wallets[0].Line 600 uses
wallets[0].chainIdto extract the chain ID. If there are multiple wallets and the active/matched wallet isn't at index 0, this will use the wrong chain.Consider applying this diff to find the active wallet first:
// Set chain ID for Privy wallets or WalletConnect if (wallets.length > 0) { - const walletChainId = +wallets[0].chainId.split(':')[1]; // extract from CAIP-2 + const activeWallet = wallets.find(w => w.address === user?.wallet?.address) ?? wallets[0]; + const walletChainId = +activeWallet.chainId.split(':')[1]; // extract from CAIP-2 const isWithinVisibleChains = visibleChains.some( (chain) => chain.id === walletChainId );
641-641: Includeconnectorsin the effect's dependency array.The effect uses
connectors.find(...)at line 419 butconnectorsis not listed in the dependency array at line 641. This can lead to stale connector references if the connectors array changes.Apply this diff:
}, [wallets, user, wagmiIsConnected, privateKey, pkAccount]); + }, [wallets, user, wagmiIsConnected, privateKey, pkAccount, connectors]);
🧹 Nitpick comments (2)
src/containers/Main.tsx (2)
252-294: Consider dynamic chain ID selection for private key authentication.The private key authentication path hardcodes
walletChainId = 1(mainnet), which may not align with the user's network preference or theisTestnetconfiguration. Consider:
- Reading the user's last selected chain from localStorage, or
- Defaulting to
isTestnet ? sepolia.id : mainnet.idfor consistency with the Privy config (line 920)Apply this diff to use the testnet-aware default:
- const walletChainId = 1; // default chain id is 1 (mainnet) + const walletChainId = isTestnet ? sepolia.id : mainnet.id; // respect testnet config
404-596: Consider dynamic chain ID for WalletConnect connections.Lines 481 and 486 hardcode mainnet (chainId = 1) for WalletConnect connections. This doesn't respect the
isTestnetconfiguration or the user's last selected network preference.Apply this diff to align with the testnet configuration:
const newProvider = createWalletClient({ account: wcAccount as `0x${string}`, - chain: getNetworkViem(1), // Default to mainnet + chain: getNetworkViem(isTestnet ? sepolia.id : mainnet.id), transport: custom(wcProvider), }); setProvider(newProvider); - setChainId(1); // Default to mainnet + setChainId(isTestnet ? sepolia.id : mainnet.id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/containers/Main.tsx(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T10:31:06.715Z
Learnt from: IAmKio
PR: pillarwallet/x#432
File: src/containers/Main.tsx:135-141
Timestamp: 2025-10-15T10:31:06.715Z
Learning: In src/containers/Main.tsx, the React Native webview messaging setup intentionally only activates when the `devicePlatform` URL parameter is present ('ios' or 'android'). On reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging. This is by design to ensure the site functions properly in both RN webview and standard browser contexts.
Applied to files:
src/containers/Main.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/containers/Main.tsx
🧬 Code graph analysis (1)
src/containers/Main.tsx (4)
src/utils/pillarWalletMessaging.ts (1)
setupPillarWalletMessaging(197-239)src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/utils/blockchain.ts (2)
visibleChains(161-163)isTestnet(33-39)src/containers/Authorized.tsx (1)
Authorized(30-162)
⏰ 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
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Bug Fixes