Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chalabi/fix mobile #157

Merged
merged 3 commits into from
Dec 16, 2024
Merged

chalabi/fix mobile #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1adfa52
fix: nice to haves
chalabi2 Dec 14, 2024
02639ba
fix: fixes mobile connections and modals
chalabi2 Dec 14, 2024
847190e
Revert "fix: nice to haves"
chalabi2 Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions components/react/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { Connected, Connecting, Error, NotExist, QRCode, WalletList, Contacts } from './views';
import { useRouter } from 'next/router';
import { ToastProvider } from '@/contexts/toastContext';

import { useDeviceDetect } from '@/hooks';
export enum ModalView {
WalletList,
QRCode,
Expand Down Expand Up @@ -46,6 +46,7 @@

const walletStatus = current?.walletStatus || WalletStatus.Disconnected;
const currentWalletName = current?.walletName;
const { isMobile } = useDeviceDetect();

useEffect(() => {
if (isOpen) {
Expand Down Expand Up @@ -89,7 +90,7 @@
setSelectedWallet(wallet);
}
if (wallet?.walletInfo.mode === 'wallet-connect') {
setCurrentView(ModalView.QRCode);
setCurrentView(isMobile ? ModalView.Connecting : ModalView.QRCode);

Check warning on line 93 in components/react/modal.tsx

View check run for this annotation

Codecov / codecov/patch

components/react/modal.tsx#L93 

Added line #L93 was not covered by tests
setQRWallet(wallet);
}
Comment on lines +93 to 95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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');
}
}

}, 1);
Comment on lines 92 to 96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Expand Down
6 changes: 4 additions & 2 deletions components/react/views/WalletList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { XMarkIcon } from '@heroicons/react/24/outline';
import { ChainWalletBase } from 'cosmos-kit';
import { getRealLogo } from '@/utils';
import { useDeviceDetect } from '@/hooks';

export const WalletList = ({
onClose,
Expand Down Expand Up @@ -29,6 +30,7 @@
wallet.walletInfo.prettyName
)
);
const { isMobile } = useDeviceDetect();

Check warning on line 33 in components/react/views/WalletList.tsx

View check run for this annotation

Codecov / codecov/patch

components/react/views/WalletList.tsx#L33 

Added line #L33 was not covered by tests
return (
<div className="p-1 relative max-w-sm mx-auto">
<h1 className="text-sm font-semibold text-center mb-6">Connect Wallet</h1>
Expand All @@ -41,7 +43,7 @@
</button>

{/* Browser and Social sections - browaer hidden on mobile/tablet */}
<div className="hidden md:block">
<div className={`${isMobile ? 'hidden' : 'block'}`}>

Check warning on line 46 in components/react/views/WalletList.tsx

View check run for this annotation

Codecov / codecov/patch

components/react/views/WalletList.tsx#L46 

Added line #L46 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 use window.matchMedia() with proper mobile device detection
  • components/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

<div className="space-y-2 mb-4">
{browser.map(({ walletInfo: { name, prettyName, logo } }) => (
<button
Expand Down Expand Up @@ -81,7 +83,7 @@
</div>

{/* Mobile Wallets Section - shown on mobile/tablet, hidden on desktop */}
<div className="md:hidden">
<div className={`${isMobile ? 'block' : 'hidden'}`}>

Check warning on line 86 in components/react/views/WalletList.tsx

View check run for this annotation

Codecov / codecov/patch

components/react/views/WalletList.tsx#L86 

Added line #L86 was not covered by tests
<div className="space-y-2">
{mobile.map(({ walletInfo: { name, prettyName, logo } }) => (
<button
Expand Down
1 change: 1 addition & 0 deletions hooks/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export * from './useQueries';
export * from './useTx';
export * from './useContacts';
export { default as useIsMobile } from './useIsMobile';
export * from './useDeviceDetect';
27 changes: 27 additions & 0 deletions hooks/useDeviceDetect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { useState, useEffect } from 'react';
declare global {
interface Navigator {
userAgentData?: {
mobile: boolean;
};
}
}
export const useDeviceDetect = () => {
const [isMobile, setIsMobile] = useState(false);

useEffect(() => {
const checkDevice = () => {
if (navigator.userAgentData) {
setIsMobile(navigator.userAgentData.mobile);

Check warning on line 15 in hooks/useDeviceDetect.tsx

View check run for this annotation

Codecov / codecov/patch

hooks/useDeviceDetect.tsx#L15 

Added line #L15 was not covered by tests
} else {
setIsMobile(
/Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent)
);
}
};

checkDevice();
}, []);
Comment on lines +12 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add SSR compatibility and cleanup

The useEffect implementation needs improvements for production reliability:

  1. Add SSR compatibility check
  2. Handle orientation changes
  3. 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.

Suggested change
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);
};
}, []);


return { isMobile };
};
Comment on lines +9 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add SSR safety checks and optimize hook implementation

The current implementation has several potential improvements:

  1. Missing SSR safety checks for window and navigator
  2. No cleanup function in useEffect
  3. checkDevice function 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.

Suggested change
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 };
};

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Add a separate script for mobile testing
  2. 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.

"build": "next build",
"start": "next start",
"lint": "next lint",
Expand Down
Loading