chalabi/fix mobile #157
chalabi/fix mobile #157
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 56.73% 56.78% +0.04%
==========================================
Files 145 146 +1
Lines 14288 14307 +19
==========================================
+ Hits 8107 8124 +17
- Misses 6181 6183 +2 ☔ View full report in Codecov by Sentry. |
This reverts commit 1adfa52.
WalkthroughThis pull request introduces a mobile device detection mechanism across the application. A new custom hook Changes
Sequence DiagramsequenceDiagram
participant Browser
participant useDeviceDetect
participant Component
Browser->>useDeviceDetect: Detect device type
useDeviceDetect-->>useDeviceDetect: Check userAgentData/userAgent
useDeviceDetect->>Component: Return isMobile state
Component->>Component: Render based on device type
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 4
🧹 Outside diff range and nitpick comments (1)
components/react/views/WalletList.tsx (1)
86-86: Consider adding a loading stateThe mobile section's visibility toggle
${isMobile ? 'block' : 'hidden'}might cause a flash of incorrect content while the device detection initializes. Consider adding a loading state.-<div className={`${isMobile ? 'block' : 'hidden'}`}> +<div className={`${isMobile === undefined ? 'hidden' : isMobile ? 'block' : 'hidden'}`}>Also applies to: 86-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/react/modal.tsx(3 hunks)components/react/views/WalletList.tsx(4 hunks)hooks/index.tsx(1 hunks)hooks/useDeviceDetect.tsx(1 hunks)package.json(1 hunks)
🔇 Additional comments (6)
hooks/useDeviceDetect.tsx (2)
2-8: LGTM: Type declaration is well-structured
The global interface extension for Navigator is correctly implemented to support the modern userAgentData API.
17-19: Consider separating tablet detection
The current regex includes tablets (iPad), which might need different UI handling than phones. Consider creating separate detection for tablets if the UI requirements differ.
Let's check if there are any tablet-specific UI implementations:
hooks/index.tsx (1)
8-9: Consider consolidating mobile detection hooks
The codebase now has two hooks for mobile detection:
- useIsMobile (existing)
- useDeviceDetect (new)
This could lead to inconsistent device detection across the application.
Consider either:
- Deprecating useIsMobile in favor of useDeviceDetect
- Having useIsMobile internally use useDeviceDetect
Let's verify the usage:
components/react/views/WalletList.tsx (1)
5-5: LGTM: Device detection implementation aligns with PR objectives
The integration of useDeviceDetect hook replaces breakpoint-based detection, which addresses the landscape mode issues mentioned in the PR objectives.
Also applies to: 33-33
components/react/modal.tsx (2)
9-9: LGTM: Device detection hook integration
The hook integration is clean and follows the component's existing patterns.
Also applies to: 49-49
93-95: Verify wallet connection flow on mobile devices
The change from QR code to direct connection view for mobile devices is a significant UX change. We should ensure:
- All mobile wallet types are handled correctly
- The transition between views is smooth
- Error states are properly managed
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the wallet connection flow, error handling, and modal view transitions.
Mobile wallet connection flow appears to be properly implemented
The codebase shows:
- Proper handling of wallet-connect mode with mobile detection
- Complete error state management (ModalView.Error for both Error and Rejected states)
- Smooth view transitions with appropriate return actions
- Clear user feedback through subtitles and status messages
- Consistent error handling patterns across different wallet states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all wallet-connect related code to verify consistent handling
rg -A 5 'wallet-connect' --type ts --type tsx
# Description: Check for potential error handling patterns we should follow
rg -A 5 'setCurrentView\(ModalView\.Error\)' --type ts --type tsx
Length of output: 169
Script:
#!/bin/bash
# Search for wallet-connect related code without file type restrictions
rg -A 5 'wallet-connect'
# Search for modal view transitions and error handling
rg -A 5 'setCurrentView\('
# Search for mobile-specific wallet handling
rg -A 5 'isMobile.*wallet'
# Look for wallet type definitions and enums
rg -A 5 'enum.*ModalView'
Length of output: 5891
| useEffect(() => { | ||
| const checkDevice = () => { | ||
| if (navigator.userAgentData) { | ||
| setIsMobile(navigator.userAgentData.mobile); | ||
| } else { | ||
| setIsMobile( | ||
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| checkDevice(); | ||
| }, []); |
There was a problem hiding this comment.
Add SSR compatibility and cleanup
The useEffect implementation needs improvements for production reliability:
- Add SSR compatibility check
- Handle orientation changes
- Include cleanup function
Apply this diff to improve the implementation:
useEffect(() => {
const checkDevice = () => {
+ if (typeof window === 'undefined') return;
if (navigator.userAgentData) {
setIsMobile(navigator.userAgentData.mobile);
} else {
setIsMobile(
/Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent)
);
}
};
checkDevice();
+ window.addEventListener('resize', checkDevice);
+
+ return () => {
+ window.removeEventListener('resize', checkDevice);
+ };
}, []);📝 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.
| useEffect(() => { | |
| const checkDevice = () => { | |
| if (navigator.userAgentData) { | |
| setIsMobile(navigator.userAgentData.mobile); | |
| } else { | |
| setIsMobile( | |
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | |
| ); | |
| } | |
| }; | |
| checkDevice(); | |
| }, []); | |
| useEffect(() => { | |
| const checkDevice = () => { | |
| if (typeof window === 'undefined') return; | |
| if (navigator.userAgentData) { | |
| setIsMobile(navigator.userAgentData.mobile); | |
| } else { | |
| setIsMobile( | |
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | |
| ); | |
| } | |
| }; | |
| checkDevice(); | |
| window.addEventListener('resize', checkDevice); | |
| return () => { | |
| window.removeEventListener('resize', checkDevice); | |
| }; | |
| }, []); |
| @@ -4,7 +4,7 @@ | |||
| "private": false, | |||
| "description": "An application to interact with the Manifest Chain", | |||
| "scripts": { | |||
| "dev": "next dev", | |||
| "dev": "next dev -H 0.0.0.0", | |||
There was a problem hiding this comment.
Document security implications of exposed dev server
While exposing the dev server to all interfaces (0.0.0.0) is useful for mobile testing, it has security implications.
Consider these improvements:
- Add a separate script for mobile testing
- Document security implications
Apply this diff:
"scripts": {
- "dev": "next dev -H 0.0.0.0",
+ "dev": "next dev",
+ "dev:mobile": "next dev -H 0.0.0.0",Add a comment in README.md:
## Mobile Development
To test on mobile devices:
1. Run `npm run dev:mobile`
2. Access using your machine's local IP address
⚠️ Security Warning: The dev:mobile script exposes the development server to all network interfaces. Use only on trusted networks and never in production.| @@ -41,7 +43,7 @@ | |||
| </button> | |||
|
|
|||
| {/* Browser and Social sections - browaer hidden on mobile/tablet */} | |||
| <div className="hidden md:block"> | |||
| <div className={`${isMobile ? 'hidden' : 'block'}`}> | |||
There was a problem hiding this comment.
💡 Codebase verification
Mobile detection needs enhancement for landscape mode
The useIsMobile hook only checks window width against a breakpoint (defaulting to 1024px) and doesn't consider device orientation. Since the conditional rendering in WalletList.tsx relies solely on this width-based check, it may not properly handle mobile devices in landscape mode where the width could exceed the breakpoint.
hooks/useIsMobile.ts: Consider enhancing the hook to usewindow.matchMedia()with proper mobile device detectioncomponents/react/views/WalletList.tsx: The current implementation might incorrectly show/hide content based only on screen width
🔗 Analysis chain
Verify browser/social section behavior in landscape mode
The conditional rendering ${isMobile ? 'hidden' : 'block'} should now correctly handle landscape mode, but we should verify this behavior.
Also applies to: 46-46
Let me gather more context about the mobile-specific visibility handling in the codebase.
Let me check the implementation of the useIsMobile hook to understand how it handles landscape mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other breakpoint-based visibility classes that might need similar updates
rg -g '*.{tsx,jsx,ts,js}' '\b(hidden|block)\s+\w+:(hidden|block)\b'
Length of output: 1541
Script:
#!/bin/bash
# Search for isMobile usage patterns in the codebase
rg -g '*.{tsx,jsx,ts,js}' 'isMobile.*?\?' -A 2
# Also check for any mobile-related hooks or utilities
rg -g '*.{tsx,jsx,ts,js}' 'useIsMobile|useMobile|isMobileDevice'
Length of output: 3855
Script:
#!/bin/bash
# Check the implementation of useIsMobile hook
cat hooks/useIsMobile.ts
# Look for any orientation-related code
rg -g '*.{tsx,jsx,ts,js}' 'orientation|landscape|screen\.orientation'
Length of output: 737
| if (wallet?.walletInfo.mode === 'wallet-connect') { | ||
| setCurrentView(ModalView.QRCode); | ||
| setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode); | ||
| setQRWallet(wallet); | ||
| } | ||
| }, 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error recovery path
The wallet connection logic should handle cases where the mobile connection fails and allow users to fall back to QR code if needed.
if (wallet?.walletInfo.mode === 'wallet-connect') {
setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode);
setQRWallet(wallet);
+ // Add timeout and error handling
+ const timeout = setTimeout(() => {
+ if (walletStatus === WalletStatus.Connecting) {
+ setCurrentView(ModalView.QRCode);
+ }
+ }, 30000); // 30 seconds timeout
+ return () => clearTimeout(timeout);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
hooks/useDeviceDetect.tsx (1)
2-8: Consider using existing TypeScript typesInstead of declaring a global interface, consider using the existing types from
@types/ua-parser-jsor similar type definitions for user agent parsing.components/react/views/WalletList.tsx (1)
46-46: Consider using Tailwind's built-in responsive classesInstead of using JavaScript-based device detection for hiding/showing sections, consider using Tailwind's responsive classes (
md:hidden,hidden md:block) which are more performant and prevent layout shifts.-<div className={`${isMobile ? 'hidden' : 'block'}`}> +<div className="hidden md:block">Also applies to: 46-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/react/modal.tsx(3 hunks)components/react/views/WalletList.tsx(4 hunks)hooks/index.tsx(1 hunks)hooks/useDeviceDetect.tsx(1 hunks)package.json(1 hunks)
🔇 Additional comments (6)
hooks/index.tsx (1)
8-9: Consider consolidating mobile detection hooks
There appears to be two hooks for mobile detection:
useIsMobile(existing)useDeviceDetect(newly added)
This could lead to inconsistent mobile detection across the application.
Let's check the implementation of both hooks:
Consider:
- Consolidating both hooks into a single, comprehensive device detection hook
- Deprecating one in favor of the other
- Documenting the specific use case for each if both are needed
package.json (1)
7-7: Security consideration for development server configuration
While binding to 0.0.0.0 allows testing on mobile devices, it exposes the development server to all network interfaces. This could pose security risks if your development machine is on a public network.
Consider adding a warning in the README about the security implications.
Recommendations:
- Document this change in the README
- Consider using environment variables to control the host binding
- Add a warning message when starting the dev server on all interfaces
Example implementation:
- "dev": "next dev -H 0.0.0.0",
+ "dev": "next dev -H ${HOST:-localhost}",
+ "dev:network": "HOST=0.0.0.0 npm run dev",components/react/views/WalletList.tsx (2)
5-5: LGTM: Device detection implementation
The useDeviceDetect hook is properly imported and integrated to determine the device type.
Also applies to: 33-33
Line range hint 86-120: Verify dark/light mode logo paths
The code assumes that social wallet logos have _light.svg and _dark.svg variants. Please ensure these files exist for all social wallets to prevent 404 errors.
✅ Verification successful
This script will help us:
- Find all SVG files that could be wallet logos
- Find code references to wallet logos with more context to understand the naming pattern
Social wallet logo variants are properly available
Based on the repository scan, all required social wallet logos with both light and dark variants are present in the /public directory:
- X (Twitter):
x_light.svg,x_dark.svg - Reddit:
reddit_light.svg,reddit_dark.svg - Google:
google_light.svg,google_dark.svg - GitHub:
github_light.svg,github_dark.svg - Discord:
discord_light.svg,discord_dark.svg - Apple:
apple_light.svg,apple_dark.svg
The code's assumption about the existence of _light.svg and _dark.svg variants for social wallet logos is correct and all necessary files are present in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all social wallet logos have both light and dark variants
rg -l "walletInfo.*logo.*svg" | while read -r file; do
# Extract logo paths from the file
rg -o 'logo.*\.svg' "$file" | while read -r logo; do
# Check if light/dark variants exist in the codebase
echo "Checking variants for: $logo"
rg "${logo}_light\.svg"
rg "${logo}_dark\.svg"
done
done
Length of output: 46
Script:
#!/bin/bash
# Let's try a different approach to find logo-related code and assets
# First, let's find all SVG files that might be wallet logos
echo "=== SVG files in the repository ==="
fd -e svg
echo -e "\n=== Code references to wallet logos ==="
# Search for logo-related code in a wider context
rg -A 5 -B 5 "walletInfo.*logo"
Length of output: 5207
components/react/modal.tsx (2)
9-9: LGTM: Device detection for modal flow
The useDeviceDetect hook is properly integrated to determine the device type.
Also applies to: 49-49
93-95: Verify mobile wallet connection flow
The changes modify the wallet connection flow for mobile devices. Please verify:
- Mobile users are correctly directed to their wallet apps
- Connection status is properly updated after returning from the wallet app
✅ Verification successful
Let me gather more information about the mobile wallet connection flow.
Based on the gathered information, I can now generate the final response:
Mobile wallet connection flow appears correctly implemented
The codebase shows proper handling of mobile wallet connections:
- Mobile users are correctly directed to the Connecting view instead of QR code view
- Connection status transitions are properly managed through WalletStatus states
- Appropriate feedback is shown to users during the connection process with mobile-specific messages
- Error states and timeouts are handled appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet connection status handling
echo "Checking wallet connection status handling:"
rg -A 5 "WalletStatus\.(Connected|Connecting|Error)"
# Search for deep linking implementation
echo "Checking deep linking implementation:"
rg -l "wallet.*connect.*mobile|universal.*link|deep.*link"
Length of output: 3188
Script:
#!/bin/bash
# Search for mobile-specific wallet connection logic
echo "Checking mobile wallet connection handling:"
rg -A 10 "isMobile.*\?" components/react/modal.tsx
# Search for QR code and mobile view handling
echo "Checking QR code and mobile view implementations:"
rg -A 5 "ModalView\.(QRCode|Connecting)"
# Search for wallet connection implementations
echo "Checking wallet connection implementations:"
ast-grep --pattern 'function $FUNC($$$) {
$$$
setCurrentView($$$)
setQRWallet($$$)
$$$
}'
Length of output: 2244
| export const useDeviceDetect = () => { | ||
| const [isMobile, setIsMobile] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const checkDevice = () => { | ||
| if (navigator.userAgentData) { | ||
| setIsMobile(navigator.userAgentData.mobile); | ||
| } else { | ||
| setIsMobile( | ||
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| checkDevice(); | ||
| }, []); | ||
|
|
||
| return { isMobile }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add SSR safety checks and optimize hook implementation
The current implementation has several potential improvements:
- Missing SSR safety checks for
windowandnavigator - No cleanup function in useEffect
checkDevicefunction could be memoized
Consider this improved implementation:
export const useDeviceDetect = () => {
const [isMobile, setIsMobile] = useState(false);
+ const checkDevice = useCallback(() => {
+ if (typeof window === 'undefined' || !window.navigator) return;
+
if (navigator.userAgentData) {
setIsMobile(navigator.userAgentData.mobile);
} else {
setIsMobile(
/Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent)
);
}
- };
+ }, []);
useEffect(() => {
- const checkDevice = () => {
- if (navigator.userAgentData) {
- setIsMobile(navigator.userAgentData.mobile);
- } else {
- setIsMobile(
- /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent)
- );
- }
- };
+ // Handle window resize for devices that can change orientation
+ window?.addEventListener('resize', checkDevice);
checkDevice();
+
+ return () => {
+ window?.removeEventListener('resize', checkDevice);
+ };
- }, []);
+ }, [checkDevice]);
return { isMobile };
};📝 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 useDeviceDetect = () => { | |
| const [isMobile, setIsMobile] = useState(false); | |
| useEffect(() => { | |
| const checkDevice = () => { | |
| if (navigator.userAgentData) { | |
| setIsMobile(navigator.userAgentData.mobile); | |
| } else { | |
| setIsMobile( | |
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | |
| ); | |
| } | |
| }; | |
| checkDevice(); | |
| }, []); | |
| return { isMobile }; | |
| }; | |
| export const useDeviceDetect = () => { | |
| const [isMobile, setIsMobile] = useState(false); | |
| const checkDevice = useCallback(() => { | |
| if (typeof window === 'undefined' || !window.navigator) return; | |
| if (navigator.userAgentData) { | |
| setIsMobile(navigator.userAgentData.mobile); | |
| } else { | |
| setIsMobile( | |
| /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) | |
| ); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| // Handle window resize for devices that can change orientation | |
| window?.addEventListener('resize', checkDevice); | |
| checkDevice(); | |
| return () => { | |
| window?.removeEventListener('resize', checkDevice); | |
| }; | |
| }, [checkDevice]); | |
| return { isMobile }; | |
| }; |
| setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode); | ||
| setQRWallet(wallet); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for QR code generation
The QR code view might fail to generate if the wallet doesn't provide a valid QR URI. Consider adding error handling:
setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode);
-setQRWallet(wallet);
+if (!isMobile) {
+ if (wallet?.qrUrl?.data) {
+ setQRWallet(wallet);
+ } else {
+ setCurrentView(ModalView.Error);
+ console.error('QR code data not available');
+ }
+}📝 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.
| setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode); | |
| setQRWallet(wallet); | |
| } | |
| setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode); | |
| if (!isMobile) { | |
| if (wallet?.qrUrl?.data) { | |
| setQRWallet(wallet); | |
| } else { | |
| setCurrentView(ModalView.Error); | |
| console.error('QR code data not available'); | |
| } | |
| } |
|
@chalabi2 Merging as-is but can you comment if there are any relevant CodeRabbit suggestions? |
Fixes the issues in #154
Summary
Mobile wallets were rendered based on breakpoints, this caused devices in landscape mode to render the wrong connection button. Moreover, mobile wallet buttons rendered qr codes rather than establishing direct connections with mobile apps. Click links to open mobile rendered regardless but this is a smoother flow that displays the connection view rather than the qr code on mobile.
Solution
Using browser api's we are actually checking if a mobile device is connected rather than relying on breakpoints.
Changes
useDetectDevicehook.wallet?.walletInfo.mode === 'wallet-connect'Summary by CodeRabbit
New Features
Bug Fixes
Documentation
useDeviceDetect, for determining if the user's device is mobile.Chores