Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes remove the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying x with
|
| Latest commit: |
94009df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1cdb03f1.x-e62.pages.dev |
| Branch Preview URL: | https://fix-metamask-walletconnect-f.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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/containers/Main.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/containers/Main.tsx (1)
Learnt from: IAmKio
PR: #243
File: src/containers/Main.tsx:85-86
Timestamp: 2025-02-20T19:50:40.447Z
Learning: Passing private key via URL parameters in the login flow is an intentional design choice for handling edge cases in the private key login mode.
⏰ 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/containers/Main.tsx (4)
97-98: Good refactoring for improved readability.The introduction of the
isPKboolean consolidates the private key login detection logic, making the subsequent conditional statements clearer and more maintainable.
100-134: Simplified condition improves code clarity.The refactoring to use the
isPKboolean and explicitly use theaccountstate in the provider creation makes the private key login flow more explicit and easier to follow.
152-189: Explicit condition improves authentication flow clarity.Converting the catch-all
elseto an explicitelse if (authenticated && wallets.length)condition makes the Privy login branch more self-documenting and easier to debug.
208-208: Correct dependency updates for authentication state reactivity.Adding
authenticatedandisConnectedto the useEffect dependencies ensures the authentication flow properly responds to state changes from both Privy and wagmi authentication systems.
github-actions
bot
temporarily deployed
to
Preview (fix/metamask-walletConnect-fix)
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 (1)
src/containers/Main.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/containers/Main.tsx (1)
Learnt from: IAmKio
PR: #243
File: src/containers/Main.tsx:85-86
Timestamp: 2025-02-20T19:50:40.447Z
Learning: Passing private key via URL parameters in the login flow is an intentional design choice for handling edge cases in the private key login mode.
⏰ 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 (5)
src/containers/Main.tsx (5)
20-20: LGTM: Import additions are correct.The
useConnectimport and its usage are properly added to support the new WalletConnect functionality.Also applies to: 71-71
98-99: Good refactoring: Improved readability withisPKvariable.Extracting the private key login condition into a single variable improves code maintainability and readability.
Also applies to: 101-101
102-102: LGTM: Private key login branch clarified.The comment and direct use of
accountstate make the private key login flow clearer and more explicit about using HTTP transport.Also applies to: 135-135
153-154: LGTM: Privy login branch made more explicit.The explicit condition check and descriptive comment clarify that this branch handles Privy-managed authentication including MetaMask, WalletConnect, and social logins.
236-236: LGTM: useEffect dependencies correctly updated.The added dependencies (
authenticated,isConnected,connectors) are all used within the effect and ensure proper re-execution when these authentication states change.
src/containers/Main.tsx
Outdated
| } else if (isConnected && address) { | ||
| // Direct WalletConnect (wagmi): use wagmi provider ONLY if not PK and not Privy | ||
| const updateProvider = async () => { | ||
| const walletChainId = 1; // default chain id is 1 | ||
| // Find the active connector (WalletConnect) | ||
| const walletConnectConnector = connectors.find( | ||
| (c) => c.id === 'walletConnect' | ||
| ); | ||
| console.log('DEBUG: walletConnectConnector', walletConnectConnector); | ||
| if (walletConnectConnector) { | ||
| const providerWagmi = await walletConnectConnector.getProvider(); | ||
| console.log('DEBUG: providerWagmi', providerWagmi); | ||
| if ( | ||
| providerWagmi && | ||
| typeof (providerWagmi as any).request === 'function' | ||
| ) { | ||
| console.log('DEBUG: Using WalletConnect provider for viem'); | ||
| const newProvider = createWalletClient({ | ||
| account: address as `0x${string}`, | ||
| chain: getNetworkViem(walletChainId), | ||
| transport: custom(providerWagmi as any), | ||
| }); | ||
| setProvider(newProvider); | ||
| const isWithinVisibleChains = visibleChains.some( | ||
| (chain) => chain.id === walletChainId | ||
| ); | ||
| setChainId( | ||
| isWithinVisibleChains ? walletChainId : visibleChains[0].id | ||
| ); | ||
| } else { | ||
| console.warn( | ||
| 'WalletConnect: connector.getProvider() returned invalid provider', | ||
| providerWagmi | ||
| ); | ||
| } | ||
| } else { | ||
| console.warn( | ||
| 'WalletConnect: walletConnectConnector not found', | ||
| connectors | ||
| ); | ||
| } | ||
| }; | ||
| updateProvider(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
WalletConnect implementation fixes the transport issue but needs cleanup.
This implementation correctly addresses the signing issue mentioned in the PR objectives by using custom(providerWagmi) transport instead of HTTP transport, ensuring signing requests are routed through the connected wallet.
However, several improvements are needed:
- Remove debug logging - Console.log statements should be removed for production
- Improve type safety - Replace
as anytype assertions with proper typing - Enhance provider validation - The EIP-1193 provider check could be more comprehensive
- console.log('DEBUG: walletConnectConnector', walletConnectConnector);
if (walletConnectConnector) {
const providerWagmi = await walletConnectConnector.getProvider();
- console.log('DEBUG: providerWagmi', providerWagmi);
if (
providerWagmi &&
- typeof (providerWagmi as any).request === 'function'
+ typeof providerWagmi.request === 'function'
) {
- console.log('DEBUG: Using WalletConnect provider for viem');
const newProvider = createWalletClient({
account: address as `0x${string}`,
chain: getNetworkViem(walletChainId),
- transport: custom(providerWagmi as any),
+ transport: custom(providerWagmi),
});Note: This implementation correctly resolves the transport issue flagged in the previous review.
📝 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.
| } else if (isConnected && address) { | |
| // Direct WalletConnect (wagmi): use wagmi provider ONLY if not PK and not Privy | |
| const updateProvider = async () => { | |
| const walletChainId = 1; // default chain id is 1 | |
| // Find the active connector (WalletConnect) | |
| const walletConnectConnector = connectors.find( | |
| (c) => c.id === 'walletConnect' | |
| ); | |
| console.log('DEBUG: walletConnectConnector', walletConnectConnector); | |
| if (walletConnectConnector) { | |
| const providerWagmi = await walletConnectConnector.getProvider(); | |
| console.log('DEBUG: providerWagmi', providerWagmi); | |
| if ( | |
| providerWagmi && | |
| typeof (providerWagmi as any).request === 'function' | |
| ) { | |
| console.log('DEBUG: Using WalletConnect provider for viem'); | |
| const newProvider = createWalletClient({ | |
| account: address as `0x${string}`, | |
| chain: getNetworkViem(walletChainId), | |
| transport: custom(providerWagmi as any), | |
| }); | |
| setProvider(newProvider); | |
| const isWithinVisibleChains = visibleChains.some( | |
| (chain) => chain.id === walletChainId | |
| ); | |
| setChainId( | |
| isWithinVisibleChains ? walletChainId : visibleChains[0].id | |
| ); | |
| } else { | |
| console.warn( | |
| 'WalletConnect: connector.getProvider() returned invalid provider', | |
| providerWagmi | |
| ); | |
| } | |
| } else { | |
| console.warn( | |
| 'WalletConnect: walletConnectConnector not found', | |
| connectors | |
| ); | |
| } | |
| }; | |
| updateProvider(); | |
| } | |
| } else if (isConnected && address) { | |
| // Direct WalletConnect (wagmi): use wagmi provider ONLY if not PK and not Privy | |
| const updateProvider = async () => { | |
| const walletChainId = 1; // default chain id is 1 | |
| // Find the active connector (WalletConnect) | |
| const walletConnectConnector = connectors.find( | |
| (c) => c.id === 'walletConnect' | |
| ); | |
| if (walletConnectConnector) { | |
| const providerWagmi = await walletConnectConnector.getProvider(); | |
| if ( | |
| providerWagmi && | |
| typeof providerWagmi.request === 'function' | |
| ) { | |
| const newProvider = createWalletClient({ | |
| account: address as `0x${string}`, | |
| chain: getNetworkViem(walletChainId), | |
| transport: custom(providerWagmi), | |
| }); | |
| setProvider(newProvider); | |
| const isWithinVisibleChains = visibleChains.some( | |
| (chain) => chain.id === walletChainId | |
| ); | |
| setChainId( | |
| isWithinVisibleChains ? walletChainId : visibleChains[0].id | |
| ); | |
| } else { | |
| console.warn( | |
| 'WalletConnect: connector.getProvider() returned invalid provider', | |
| providerWagmi | |
| ); | |
| } | |
| } else { | |
| console.warn( | |
| 'WalletConnect: walletConnectConnector not found', | |
| connectors | |
| ); | |
| } | |
| }; | |
| updateProvider(); | |
| } |
🤖 Prompt for AI Agents
In src/containers/Main.tsx between lines 191 and 234, remove all console.log
debug statements to clean up the code for production. Replace the `as any` type
assertions on providerWagmi with proper EIP-1193 provider typings to improve
type safety. Enhance the provider validation by implementing a more
comprehensive check for EIP-1193 compliance, such as verifying the presence of
required methods like request, on, and removeListener, instead of only checking
if request is a function.
github-actions
bot
temporarily deployed
to
Preview (fix/metamask-walletConnect-fix)
github-actions
bot
temporarily deployed
to
Preview (fix/metamask-walletConnect-fix)
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/pages/Login.tsx (1)
34-47: Add error handling for logout operation.If the
logout()call fails, the WalletConnect setup will still proceed, which could lead to inconsistent authentication state. Consider adding error handling.const handleWalletConnect = async () => { - await logout(); // Logout Privy session if any + try { + await logout(); // Logout Privy session if any + } catch (error) { + console.warn('Failed to logout from Privy:', error); + } if (!walletConnectConnector) { throw new Error('WalletConnect connector not found'); } connect({ connector: walletConnectConnector });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/BottomMenuModal/AccountModal.tsx(1 hunks)src/pages/Loading.tsx(3 hunks)src/pages/Login.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/pages/Login.tsx (1)
src/components/LandingPage/Button/index.jsx (1)
Button(38-61)
⏰ 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/pages/Loading.tsx (3)
7-7: LGTM: Import addition aligns with authentication flow improvements.The addition of
useDisconnectimport is consistent with the broader changes to improve session management across authentication methods.
18-18: LGTM: Hook usage is correct.The
useDisconnecthook is properly destructured and will be used in the auto-logout flow.
52-52: LGTM: Dependency array correctly updated.The addition of
disconnectto the dependency array ensures the effect will update if the disconnect function changes.src/components/BottomMenuModal/AccountModal.tsx (1)
153-160: LGTM: Unified logout logic improves consistency.The refactored logic ensures both
disconnect()andlogout()are called together when either a wagmi connection or Privy account exists. This eliminates the previous separate handling and aligns with the consistent session cleanup pattern implemented across other components.The change is safe because both functions handle cases where there's nothing to disconnect/logout from gracefully.
src/pages/Login.tsx (2)
2-2: LGTM: Import additions support the authentication flow improvements.The added imports (
useLogout,useDisconnect) are necessary for the enhanced session management functionality.Also applies to: 4-4, 7-7, 14-14, 21-22
78-78: LGTM: Button handlers correctly use new authentication functions.The button click handlers properly reference the new
handlePrivyLoginandhandleWalletConnectfunctions, ensuring proper session management when switching between authentication methods.Also applies to: 81-81
| const handleLogout = async () => { | ||
| await disconnect(); | ||
| await logout(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the disconnect operation.
While the implementation correctly calls disconnect() before logout(), there's no error handling if the disconnect operation fails. This could prevent the logout from occurring.
const handleLogout = async () => {
+ try {
await disconnect();
+ } catch (error) {
+ console.warn('Failed to disconnect:', error);
+ }
await logout();
};📝 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 handleLogout = async () => { | |
| await disconnect(); | |
| await logout(); | |
| }; | |
| const handleLogout = async () => { | |
| try { | |
| await disconnect(); | |
| } catch (error) { | |
| console.warn('Failed to disconnect:', error); | |
| } | |
| await logout(); | |
| }; |
🤖 Prompt for AI Agents
In src/pages/Loading.tsx around lines 44 to 47, the handleLogout function calls
disconnect() without error handling, which could block logout if disconnect
fails. Wrap the disconnect() call in a try-catch block to catch any errors, log
or handle them appropriately, and ensure that logout() is called regardless of
disconnect's success or failure.
src/pages/Login.tsx
Outdated
| const handlePrivyLogin = async () => { | ||
| disconnect(); // Disconnect wagmi session if any | ||
| await login(); // Then login with Privy | ||
| }; |
There was a problem hiding this comment.
Fix async handling in handlePrivyLogin.
The disconnect() call should be awaited to ensure the wagmi session is properly disconnected before starting the Privy login flow. This prevents potential race conditions.
const handlePrivyLogin = async () => {
- disconnect(); // Disconnect wagmi session if any
+ await disconnect(); // Disconnect wagmi session if any
await login(); // Then login with Privy
};📝 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 handlePrivyLogin = async () => { | |
| disconnect(); // Disconnect wagmi session if any | |
| await login(); // Then login with Privy | |
| }; | |
| const handlePrivyLogin = async () => { | |
| await disconnect(); // Disconnect wagmi session if any | |
| await login(); // Then login with Privy | |
| }; |
🤖 Prompt for AI Agents
In src/pages/Login.tsx around lines 29 to 32, the disconnect() function call
inside handlePrivyLogin is not awaited, which can cause race conditions by
starting login before disconnect completes. Fix this by adding the await keyword
before disconnect() to ensure the wagmi session is fully disconnected before
proceeding with the login call.
github-actions
bot
temporarily deployed
to
Preview (fix/metamask-walletConnect-fix)
This reverts commit bc920ff.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Bug Fixes
Style