fix: error page for non-MM users #1696
Conversation
WalkthroughThe updates involve enhancing the user experience for connecting a wallet in a web application. The changes include a new import of an Changes
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tools/obscuroscan_v3/frontend/pages/personal/index.tsx (2 hunks)
- tools/obscuroscan_v3/frontend/src/components/modules/common/connect-wallet.tsx (2 hunks)
- tools/obscuroscan_v3/frontend/src/components/providers/wallet-provider.tsx (3 hunks)
- tools/obscuroscan_v3/frontend/src/lib/utils.ts (1 hunks)
Additional comments: 13
tools/obscuroscan_v3/frontend/pages/personal/index.tsx (4)
8-8: The import of
ethereumfrom@/src/lib/utilsis correctly added to support the conditional rendering logic in theEmptyStatecomponent'sactionprop.24-24: The update to the
EmptyStatecomponent'stitleprop from "Connect your wallet" to "Connect Wallet" is a straightforward text change that improves consistency and clarity.26-34: The conditional rendering logic within the
EmptyStatecomponent'sactionprop is correctly implemented. It uses the presence of theethereumobject to determine the text displayed on theConnectWalletButton, providing a clear message to the user based on whether MetaMask is installed.27-33: Verify that the
ConnectWalletButtoncomponent has been updated to accept and properly handle the newtextprop.tools/obscuroscan_v3/frontend/src/components/modules/common/connect-wallet.tsx (5)
6-7: The addition of the
textprop to theConnectWalletButtoncomponent is consistent with the PR's objective to provide clearer messaging for users without MetaMask.15-21: The updated
onClickhandler logic now correctly handles the presence or absence ofethereum, providing a fallback action to download MetaMask when it's not present. This change should prevent theTypeErrornon-MetaMask users were experiencing.36-36: The conditional rendering of the button text using the
textprop or a default value is a good user experience improvement, making it clear to users what action is expected of them.6-6: Importing the
ethereumobject fromutils.tsis a good practice for maintainability and testability, as it centralizes the access toethereumand abstracts away the direct dependency on the globalwindowobject.22-22: Verify the need for the
suppressHydrationWarningprop on theButtoncomponent to ensure it's being used intentionally and not masking a potential issue with server/client markup mismatch.tools/obscuroscan_v3/frontend/src/components/providers/wallet-provider.tsx (4)
9-9: The import of
ethereumfromutilsis a good practice to centralize the access to theethereumobject and handle its absence in a single place.33-34: The conditional check for
ethereumbefore creating a newWeb3Provideris a good addition to prevent errors for non-MetaMask users.67-70: The early return in the
useEffecthook ifethereumis not present is a good defensive programming practice.79-83: Setting up and removing the
accountsChangedevent listener is correctly handled with the presence check forethereum.
| export const { ethereum } = | ||
| typeof window !== "undefined" ? window : ({} as any); |
There was a problem hiding this comment.
The ethereum export is incorrectly assigned the entire window object instead of window.ethereum.
- export const { ethereum } =
- typeof window !== "undefined" ? window : ({} as any);
+ export const ethereum =
+ typeof window !== "undefined" ? window.ethereum : ({} as any);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.
| export const { ethereum } = | |
| typeof window !== "undefined" ? window : ({} as any); | |
| export const ethereum = | |
| typeof window !== "undefined" ? window.ethereum : ({} as any); |
| export const downloadMetaMask = () => { | ||
| window ? window.open("https://metamask.io/download", "_blank") : null; |
There was a problem hiding this comment.
The downloadMetaMask function should check for the existence of window.open before attempting to call it.
- window ? window.open("https://metamask.io/download", "_blank") : null;
+ if (window && typeof window.open === 'function') {
+ window.open("https://metamask.io/download", "_blank");
+ }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.
| export const downloadMetaMask = () => { | |
| window ? window.open("https://metamask.io/download", "_blank") : null; | |
| export const downloadMetaMask = () => { | |
| if (window && typeof window.open === 'function') { | |
| window.open("https://metamask.io/download", "_blank"); | |
| } |
| @@ -3,15 +3,23 @@ import { Button } from "@/src/components/ui/button"; | |||
| import { Link2Icon, LinkBreak2Icon } from "@radix-ui/react-icons"; | |||
| import React from "react"; | |||
| import TruncatedAddress from "./truncated-address"; | |||
| const ConnectWalletButton = () => { | |||
| import { downloadMetaMask, ethereum } from "@/src/lib/utils"; | |||
| const ConnectWalletButton = ({ text }: { text?: string }) => { | |||
There was a problem hiding this comment.
Consider adding documentation or comments explaining the purpose and expected values for the text prop to ensure clarity for other developers.
| @@ -10,3 +10,10 @@ | |||
| const date = new Date(Number(unixTimestampSeconds) * 1000); | |||
| return formatDistanceToNow(date, { addSuffix: true }); | |||
| } | |||
|
|
|||
| export const { ethereum } = | |||
| typeof window !== "undefined" ? window : ({} as any); | |||
There was a problem hiding this comment.
The fallback type for ethereum when window is not defined is any, which is not type-safe. Consider defining a more specific type or interface for the ethereum object.
- typeof window !== "undefined" ? window : ({} as any);
+ typeof window !== "undefined" ? window.ethereum : ({} as EthereumType);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.
| typeof window !== "undefined" ? window : ({} as any); | |
| typeof window !== "undefined" ? window.ethereum : ({} as EthereumType); |
| typeof window !== "undefined" ? window : ({} as any); | ||
|
|
||
| export const downloadMetaMask = () => { | ||
| window ? window.open("https://metamask.io/download", "_blank") : null; |
There was a problem hiding this comment.
The downloadMetaMask function uses a ternary operator unnecessarily. Simplify this to an if-statement for better readability.
- window ? window.open("https://metamask.io/download", "_blank") : null;
+ if (window) {
+ window.open("https://metamask.io/download", "_blank");
+ }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.
| window ? window.open("https://metamask.io/download", "_blank") : null; | |
| if (window) { | |
| window.open("https://metamask.io/download", "_blank"); | |
| } |
| (window as any).ethereum | ||
| ); | ||
| if (ethereum) { | ||
| const ethProvider = new ethers.providers.Web3Provider(ethereum); | ||
| setProvider(ethProvider); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Consider providing a more informative error message or guidance in the else block of the connectWallet function for users without MetaMask.
showToast(
ToastType.DESTRUCTIVE,
- "No ethereum object found. Please install MetaMask!"
+ "MetaMask is not detected. Please install MetaMask to connect your wallet, or visit [link to guide] for more information."
);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.
| try { | |
| try { |
Jennievon
deleted the
jennifer/2594-tenscan-throws-error-for-users-without-mm
branch
Why this change is needed
Please provide a description and a link to the underlying ticket
https://github.com/ten-protocol/ten-internal/issues/2594
What changes were made as part of this PR
Please provide a high level list of the changes made
Error message for non-MM users:
main-12b4344bfd88e8af.js:1 TypeError: Cannot read properties of undefined (reading 'on')PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks