feat: switch network instead of disconnect #929
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes made in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
hooks/isWrongNetwork.tsx (2)
17-23: Consider adding custom network mismatch messagesTo better align with the PR's UX improvement goals, consider adding custom error messages that clearly indicate which network the user should switch to.
setIsWrongNetwork( process.env.NEXT_PUBLIC_IS_TESTNET === "true" ? bigintToStringHex(chain.id) === bigintToStringHex(mainnet.id) : bigintToStringHex(chain.id) === bigintToStringHex(sepolia.id) ); + // Consider adding: + const targetNetwork = process.env.NEXT_PUBLIC_IS_TESTNET === "true" ? sepolia : mainnet; + setNetworkMessage(`Please switch to ${targetNetwork.name}`);
Line range hint
1-32: Consider adding error handling for network switching failuresTo ensure a robust user experience, consider implementing error handling for cases where network switching fails (e.g., user rejection, RPC errors). This could be done by:
- Adding an error state to track switching failures
- Providing feedback to users when switching fails
- Implementing retry logic if needed
Would you like me to provide a detailed implementation suggestion for the error handling?
components/UI/navbar.tsx (2)
393-394: Enhance UX with loading state during network switchConsider adding a loading state to provide feedback while the network switch is in progress.
+ const [isSwitching, setIsSwitching] = useState(false); + const handleNetworkSwitch = async () => { + setIsSwitching(true); + try { + await switchNetwork(); + } finally { + setIsSwitching(false); + } + }; - <Button onClick={() => switchNetwork(network)}> + <Button + onClick={handleNetworkSwitch} + disabled={isSwitching} + > + {isSwitching ? 'Switching...' : `Switch to ${network}`} </Button>
62-69: Consider extracting network switching logic to a custom hookThe network switching logic could be extracted into a custom hook (e.g.,
useNetworkSwitch) to:
- Improve reusability across components
- Centralize error handling and loading states
- Separate concerns from the UI component
Example implementation:
// hooks/useNetworkSwitch.ts export const useNetworkSwitch = () => { const [isSwitching, setIsSwitching] = useState(false); const network = process.env.NEXT_PUBLIC_IS_TESTNET === "true" ? "testnet" : "mainnet"; const { switchChainAsync } = useSwitchChain({ params: { chainId: network === "testnet" ? StarknetChainId.SN_SEPOLIA : StarknetChainId.SN_MAIN, }, }); const switchNetwork = async () => { setIsSwitching(true); try { const res = await switchChainAsync(); return res; } catch (error) { console.error('Failed to switch network:', error); throw error; } finally { setIsSwitching(false); } }; return { switchNetwork, isSwitching, network }; };Also applies to: 148-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/UI/navbar.tsx(5 hunks)hooks/isWrongNetwork.tsx(1 hunks)
🔇 Additional comments (4)
hooks/isWrongNetwork.tsx (3)
4-4: LGTM: Import addition aligns with new functionality
The addition of useAccount import is appropriate for the new account-aware network switching functionality.
9-9: LGTM: Account state management added
Good addition of account state tracking to prevent unnecessary network validation when no account is connected.
13-16: Verify account switching behavior
The early return logic is well implemented, but let's verify the behavior during account switches to ensure proper state updates.
✅ Verification successful
Early return logic is correctly implemented and safe for account switching
The implementation is correct and properly handles account switching scenarios:
- The hook correctly updates
isWrongNetworkstate when accounts change through the dependency array[account, chain] - The early return with
isWrongNetworkset tofalsewhen no account is present is the desired behavior - Components using this hook (
navbar.tsxandfreeRegisterCheckout.tsx) properly handle the network status changes - The state updates are synchronized with account changes through React's useEffect
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper handling of account switching scenarios
# Look for other components that might depend on this hook's behavior during account switches
# Search for components using this hook
rg -l "useIsWrongNetwork" --type tsx --type ts
# Look for account switching logic that might interact with this hook
ast-grep --pattern 'useAccount()' -l
Length of output: 219
Script:
#!/bin/bash
# Search for components using this hook
rg -l "useIsWrongNetwork"
# Look for account switching logic that might interact with this hook
rg -l "useAccount"
# Get the implementation details of the hook
cat hooks/isWrongNetwork.tsx
# Look for components that might be affected by network status
rg "isWrongNetwork" -A 3
Length of output: 5447
components/UI/navbar.tsx (1)
13-18: LGTM: Import statements are properly organized
The addition of useSwitchChain and StarknetChainId imports aligns well with the PR's objective of implementing network switching functionality.
Also applies to: 29-29
components/UI/navbar.tsx
Outdated
| const switchNetwork = async (network: string) => { | ||
| const res = await switchChainAsync(); | ||
| if (res) setIsWrongNetwork(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve switchNetwork function implementation
The function has a few areas for improvement:
- The
networkparameter is unused - Missing error handling for failed network switches
- const switchNetwork = async (network: string) => {
+ const switchNetwork = async () => {
+ try {
const res = await switchChainAsync();
if (res) setIsWrongNetwork(false);
+ } catch (error) {
+ console.error('Failed to switch network:', error);
+ // Consider showing an error message to the user
+ }
};📝 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 switchNetwork = async (network: string) => { | |
| const res = await switchChainAsync(); | |
| if (res) setIsWrongNetwork(false); | |
| }; | |
| const switchNetwork = async () => { | |
| try { | |
| const res = await switchChainAsync(); | |
| if (res) setIsWrongNetwork(false); | |
| } catch (error) { | |
| console.error('Failed to switch network:', error); | |
| // Consider showing an error message to the user | |
| } | |
| }; |
fricoben
added
❌ Change request
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
utils/connectorWrapper.ts (1)
94-94: Add JSDoc documentation for better code maintainability.Consider adding documentation to explain the function's purpose and parameters:
+/** + * Checks if the given connector is an Argent wallet + * @param connector - Optional StarknetReactConnector instance + * @returns true if the connector is an Argent wallet, false otherwise + */ export const isArgentWallet = (connector?: StarknetReactConnector) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/UI/navbar.tsx(6 hunks)utils/connectorWrapper.ts(2 hunks)
🔇 Additional comments (6)
components/UI/navbar.tsx (3)
13-18: LGTM: Import statements are properly organized
The new imports for network switching functionality are correctly added and organized.
Also applies to: 29-29, 35-35
63-70: Add error handling for switchChainAsync
While the chain configuration is correct, consider adding error handling for cases where the network switch fails.
149-155: 🛠️ Refactor suggestion
Enhance network switching implementation
The implementation needs improvements in several areas:
- Add error handling for network switching
- Add loading state during network switch
- Add TypeScript return type annotation
- const switchNetwork = async () => {
+ const switchNetwork = async (): Promise<void> => {
+ try {
if (isArgentWallet(connector)) disconnectByClick();
else {
+ setIsWrongNetwork(true); // Show loading state
const res = await switchChainAsync();
if (res) setIsWrongNetwork(false);
}
+ } catch (error) {
+ console.error('Network switch failed:', error);
+ // Consider showing an error message to the user
+ } finally {
+ setIsWrongNetwork(false); // Reset loading state
+ }
};Likely invalid or redundant comment.
utils/connectorWrapper.ts (3)
7-7: LGTM!
The import of StarknetReactConnector is necessary for type safety in the new isArgentWallet function.
94-101: LGTM! Implementation is correct and aligns with PR objectives.
The function correctly identifies Argent wallets by checking the connector's ID against all known Argent wallet types.
94-101: Verify the network switching implementation.
Let's verify that the network switching logic correctly uses this function for all Argent wallet types.
✅ Verification successful
Let me try a different search strategy to verify the network switching implementation.
Based on the search results, I can now provide a final response:
Network switching implementation is correctly handling Argent wallets
The implementation in components/UI/navbar.tsx shows that the isArgentWallet function is properly used in the network switching logic:
- For Argent wallets (argentX, argentMobile, argentWebWallet), it triggers
disconnectByClick() - For other wallets, it uses
switchChainAsync()
The UI also correctly displays different text based on the wallet type:
<Button onClick={() => switchNetwork()}>
{isArgentWallet(connector)
? `Switch to ${network}`
: "Disconnect"}
</Button>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of isArgentWallet function in network switching logic
# and verify consistent handling of all Argent wallet types.
# Check for network switching implementation using isArgentWallet
echo "Checking network switching implementation..."
rg -A 5 "isArgentWallet.*switchNetwork"
# Verify all Argent wallet types are handled
echo "Verifying Argent wallet types handling..."
ast-grep --pattern $'isArgentWallet\(.*\) {
return (
$$$
(connector.id === "argentX" ||
connector.id === "argentMobile" ||
connector.id === "argentWebWallet")
$$$
);
}'
Length of output: 431
Script:
#!/bin/bash
# Let's search for network switching related code and its connection to Argent wallets
# Search for network switching implementations
echo "Searching for network switching implementations..."
rg -A 10 "switchNetwork|changeNetwork"
# Search for any usage of isArgentWallet function
echo "Searching for isArgentWallet usage..."
rg -A 5 "isArgentWallet"
# Search for Argent wallet related network handling
echo "Searching for Argent wallet network handling..."
rg -A 10 "argentX|argentMobile|argentWebWallet.*network"
Length of output: 22083
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/UI/navbar.tsx (1)
Line range hint
149-401: Consider implementing a more robust network switching strategyWhile the current implementation achieves the basic objective of switching networks, consider these architectural improvements:
- Create a custom hook (e.g.,
useNetworkSwitch) to encapsulate the network switching logic, making it reusable across components- Implement a proper state machine for network switching states (idle, switching, success, error)
- Add analytics to track successful/failed network switches for better monitoring
This would make the implementation more maintainable and provide better insights into user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/UI/navbar.tsx(6 hunks)
🔇 Additional comments (4)
components/UI/navbar.tsx (4)
13-18: LGTM: Import statements are well-organized
The new imports (useSwitchChain, StarknetChainId, isArgentWallet) are correctly added and align with the network switching functionality.
Also applies to: 29-29, 35-35
63-70: Add error handling for switchChainAsync
The hook setup is correct, but error handling is still missing as mentioned in a previous review.
149-156: Improve switchNetwork implementation
The function implementation needs improvements as mentioned in previous reviews:
- Error handling is missing
- Loading state should be added
- The network parameter is unused
398-401: 🛠️ Refactor suggestion
Improve button text clarity and add loading state
- The "Disconnect" text for non-Argent wallets doesn't accurately reflect the action (switching networks). This could be confusing for users.
- As mentioned in a previous review, consider adding a loading state during network switching.
Additionally, consider adding a confirmation dialog before disconnecting non-Argent wallets to improve user experience:
const switchNetwork = async () => {
if (isArgentWallet(connector)) {
const res = await switchChainAsync();
if (res) setIsWrongNetwork(false);
} else {
+ const confirmed = await confirm(
+ "This wallet doesn't support automatic network switching. Would you like to disconnect?"
+ );
+ if (confirmed) {
disconnectByClick();
+ }
}
};
irisdv
added
🔥 Ready for review
utils/connectorWrapper.ts
Outdated
| @@ -90,6 +91,15 @@ export const getLastConnected = (): Connector | null => { | |||
| return null; | |||
| }; | |||
|
|
|||
| export const isArgentWallet = (connector?: StarknetReactConnector) => { | |||
There was a problem hiding this comment.
Testé sur Sepolia, ne marche pas pour mobile + webwallet.
Je propose de renommer isArgentExtension et ref :)
There was a problem hiding this comment.
Ah j'ai renommé en supportSwitchNetwork comme ça on pourra mettre à jour la fonction dès que d'autres wallet ajoutent le support
fricoben
added
❌ Change request
irisdv
added
🔥 Ready for review
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/UI/navbar.tsx(6 hunks)utils/connectorWrapper.ts(2 hunks)
🔇 Additional comments (6)
components/UI/navbar.tsx (4)
13-18: LGTM: Import statements are well-organized
The new imports are appropriate for implementing the network switching functionality.
Also applies to: 29-29, 35-35
398-401: Improve button UI and loading state
The button should show a loading state during network switching and maintain consistent messaging.
- <Button onClick={() => switchNetwork()}>
- {supportSwitchNetwork(connector)
- ? `Switch to ${network}`
- : "Disconnect"}
+ <Button
+ onClick={() => switchNetwork()}
+ disabled={isNetworkSwitching}
+ >
+ {isNetworkSwitching ? (
+ <CircularProgress size={20} />
+ ) : (
+ supportSwitchNetwork(connector)
+ ? `Switch to ${network}`
+ : `Switch to ${network}`
+ )}
</Button>63-70: 🛠️ Refactor suggestion
Add error handling for network switching
While the chain configuration is correct, consider adding error handling for network switch failures.
const { switchChainAsync } = useSwitchChain({
params: {
chainId:
network === "testnet"
? StarknetChainId.SN_SEPOLIA
: StarknetChainId.SN_MAIN,
},
+ onError: (error) => {
+ console.error('Failed to switch network:', error);
+ // Consider showing an error message to the user
+ }
});149-156: 🛠️ Refactor suggestion
Enhance network switching implementation
The implementation needs error handling and loading state management.
+ const [isNetworkSwitching, setIsNetworkSwitching] = useState<boolean>(false);
const switchNetwork = async () => {
+ setIsNetworkSwitching(true);
try {
if (supportSwitchNetwork(connector)) {
const res = await switchChainAsync();
if (res) setIsWrongNetwork(false);
} else {
disconnectByClick();
}
+ } catch (error) {
+ console.error('Failed to switch network:', error);
+ // Consider showing an error message to the user
+ } finally {
+ setIsNetworkSwitching(false);
}
};utils/connectorWrapper.ts (2)
7-7: LGTM!
The import with alias is well-structured to avoid naming conflicts with the existing Connector type.
94-96: Verify integration with network switching UI
Let's verify the usage of supportSwitchNetwork function across the codebase.
✅ Verification successful
Integration with network switching UI is properly implemented
The verification shows that supportSwitchNetwork is correctly integrated:
- It's imported and used in
components/UI/navbar.tsx - Used in the
switchNetworkfunction to determine whether to switch chains or disconnect - Properly controls the button text to show either "Switch to {network}" or "Disconnect" based on wallet capability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of supportSwitchNetwork function and network switching UI integration
# Check for usage of supportSwitchNetwork
echo "Checking usage of supportSwitchNetwork function..."
rg -A 5 "supportSwitchNetwork"
# Check for network switching UI components
echo "Checking network switching UI components..."
rg -A 5 "switchNetwork|changeNetwork|networkSwitch"
Length of output: 2431
| export const supportSwitchNetwork = (connector?: StarknetReactConnector) => { | ||
| return connector && connector.id === "argentX"; | ||
| }; |
There was a problem hiding this comment.
Review network switching support for all wallet types
The current implementation has several concerns:
- It only supports argentX wallet, which may be too restrictive.
- Past review comments indicate issues with mobile + webwallet on Sepolia network.
- The function lacks documentation about its purpose and limitations.
Consider:
- Adding support for other wallets that can switch networks.
- Adding documentation about supported wallets and networks.
- Addressing the mobile + webwallet issues on Sepolia.
+/**
+ * Checks if the given connector supports network switching.
+ * Currently supported wallets:
+ * - argentX
+ *
+ * @param connector The Starknet React connector to check
+ * @returns true if the connector supports network switching
+ */
export const supportSwitchNetwork = (connector?: StarknetReactConnector) => {
- return connector && connector.id === "argentX";
+ return connector && (
+ connector.id === "argentX"
+ // TODO: Add support for other wallets that can switch networks
+ );
};📝 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.
| export const supportSwitchNetwork = (connector?: StarknetReactConnector) => { | |
| return connector && connector.id === "argentX"; | |
| }; | |
| /** | |
| * Checks if the given connector supports network switching. | |
| * Currently supported wallets: | |
| * - argentX | |
| * | |
| * @param connector The Starknet React connector to check | |
| * @returns true if the connector supports network switching | |
| */ | |
| export const supportSwitchNetwork = (connector?: StarknetReactConnector) => { | |
| return connector && ( | |
| connector.id === "argentX" | |
| // TODO: Add support for other wallets that can switch networks | |
| ); | |
| }; |
Close #127
Summary by CodeRabbit
New Features
Bug Fixes
Documentation