fix: Add alert for missing wallet in Switch Chain modal #207
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdded a runtime guard in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![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
🧹 Nitpick comments (3)
client/app/create/page.tsx (3)
27-27: Remove the development comment.The "TRAINER FIX" prefix appears to be a development or training artifact that should not be in production code.
🔎 Suggested change
- // TRAINER FIX: Check if a Wallet is actually installed first! + // Check if a crypto wallet is installed
28-28: Consider adding proper typing for window.ethereum.Using
(window as any).ethereumbypasses type checking. Consider declaring a proper type for better type safety.🔎 Suggested type declaration
Add this type declaration at the top of the file or in a types file:
declare global { interface Window { ethereum?: any; } }Then update the check:
- if (typeof window !== "undefined" && !(window as any).ethereum) { + if (typeof window !== "undefined" && !window.ethereum) {
29-31: Consider improving the user experience.The current implementation has a few UX concerns:
Double notification: Using both
toast.error()andalert()is redundant. The blockingalert()modal interrupts the user flow and is generally considered poor UX in modern web applications.MetaMask-specific messaging: While MetaMask is popular,
window.ethereumcan be provided by other wallets (Coinbase Wallet, Brave Wallet, etc.). The error message should be more generic.Popup blocker risk:
window.open()might be blocked by popup blockers in some browsers.🔎 Suggested improvements
- toast.error("No Crypto Wallet Found!"); - alert("Please install MetaMask to continue."); - window.open("https://metamask.io/download/", "_blank"); + toast.error( + "No crypto wallet detected. Please install a wallet extension (e.g., MetaMask) to continue.", + { duration: 6000 } + ); + // Optionally provide a link in the UI instead of opening automaticallyAlternatively, if you prefer to keep the automatic redirect:
- toast.error("No Crypto Wallet Found!"); - alert("Please install MetaMask to continue."); + toast.error("No crypto wallet detected. Redirecting to wallet installation page..."); window.open("https://metamask.io/download/", "_blank");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
client/app/create/page.tsx(1 hunks)client/package.json(1 hunks)
🔇 Additional comments (1)
client/app/create/page.tsx (1)
26-37: Good fix for the silent failure issue!The wallet detection logic successfully addresses the main problem from issue #206 by:
- Adding an SSR-safe check for
window.ethereum- Providing user feedback instead of failing silently
- Preventing the chain switch attempt when no wallet is detected
- Using an early return to avoid propagating errors
The implementation correctly solves the reported bug. The suggestions above are refinements to improve UX and code quality.
| "@floating-ui/core": "^1.7.3", | ||
| "@floating-ui/dom": "^1.7.4", | ||
| "@floating-ui/react": "^0.27.16", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for floating-ui imports and usage in the client code
echo "=== Searching for @floating-ui imports ==="
rg -n '@floating-ui' --type=ts --type=tsx
echo ""
echo "=== Searching for useFloating hook usage ==="
rg -n 'useFloating' --type=ts --type=tsx
echo ""
echo "=== Summary: Checking package.json to confirm packages are listed ==="
grep -n '@floating-ui' client/package.jsonRepository: AOSSIE-Org/Agora-Blockchain
Length of output: 419
Remove unused floating-ui dependencies.
These packages were added to package.json but are not imported or used anywhere in the codebase. They increase bundle size without providing value. If floating elements (tooltips, popovers, etc.) are planned for future use, they can be added when actually needed.
🤖 Prompt for AI Agents
In client/package.json around lines 12 to 14, the three @floating-ui entries are
unused; remove the "@floating-ui/core", "@floating-ui/dom", and
"@floating-ui/react" dependencies from package.json, run your package manager to
update node_modules and the lockfile (npm install / yarn install / pnpm
install), run the test/build to ensure nothing breaks, and commit the updated
package.json and lockfile changes.
|
Hi @mentors! I've submitted this PR to fix the 'Switch Chain' silent failure. I've verified the fix locally by disabling the wallet extension to simulate a new user environment. Note: I've also joined the Discord (Username: Sai Suraj), but I'm currently unable to post messages there due to new-member restrictions. I'll keep an eye on this thread for any reviews or feedback. Thanks! |
1 participant
Description
Currently, clicking the "Switch Chain" button fails silently if the user does not have a crypto wallet (like MetaMask) installed. This PR adds a check to verify if
window.ethereumexists. If not, it alerts the user and guides them to install a wallet.Type of Change
How Has This Been Tested?
Checklist:
Closes #206
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.