feat: sort the already authorized accounts to the bottom of the list to easier auth the unauthorized accounts #1662
Conversation
WalkthroughThe changes in this pull request involve multiple components within the PolkaGate extension. Key updates include the introduction of a sorting mechanism in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)packages/extension-polkagate/src/popup/authorize/Request.tsx (1)
The addition of 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: 1
🧹 Outside diff range and nitpick comments (8)
packages/extension-polkagate/src/partials/ConnectedDappIcon.tsx (2)
27-27: Consider reducing animation delay for better UXThe current 1-second delay before animation might feel sluggish to users. Consider reducing the delay to 300-500ms for a more responsive feel while keeping the 1s duration for smooth animation.
- const flip = useAnimateOnce(Boolean(favIconUrl), { delay: 1000, duration: 1000 }); + const flip = useAnimateOnce(Boolean(favIconUrl), { delay: 300, duration: 1000 });
Line range hint
31-54: Consider user feedback for error statesWhile the error is logged to console, users might benefit from visual feedback when tab checking fails.
Consider adding a subtle visual indicator or tooltip for error states:
} catch (error) { console.error('Error checking tab:', error); setIsConnected(false); + // Optional: Set error state for user feedback + setFavIconUrl('/assets/error-icon.png'); // Or use a default error icon } finally { setChecking(false); }packages/extension-polkagate/src/popup/authManagement/index.tsx (2)
Line range hint
40-56: LGTM! Good use of React.memo for performance optimization.The memoization is well-applied here as the component's rendering depends purely on its props. The inner function naming follows React best practices.
Consider adding a custom comparison function to
React.memoif the props comparison needs to be more specific than shallow equality:-const ExtensionMode = React.memo(function ExtensionMode ({ dappInfo, onBackClick, setDappInfo }: ExtensionModeType) { +const ExtensionMode = React.memo(function ExtensionMode ({ dappInfo, onBackClick, setDappInfo }: ExtensionModeType) { // ... component implementation -}); +}, (prevProps, nextProps) => { + return prevProps.dappInfo?.id === nextProps.dappInfo?.id; +});
Line range hint
78-115: Consider if memoization is necessary for this component.The AuthManagement component might not benefit significantly from memoization because:
- It manages internal state (dappInfo)
- Uses hooks that may trigger re-renders
- Has minimal props (only open and setDisplayPopup)
Consider these improvements:
- Remove the memoization if it doesn't provide measurable performance benefits
- Split the component into smaller, more focused components
- Move the data fetching logic into a custom hook
-export default React.memo(AuthManagement); +export default AuthManagement;Additionally, consider extracting the data fetching logic:
function useAuthInfo(dappId: string | undefined) { const [dappInfo, setDappInfo] = useState<AuthUrlInfo | undefined>(); useEffect(() => { if (dappId) { getAuthList() .then(({ list: authList }) => { if (dappId in authList) { setDappInfo(authList[dappId]); } }) .catch(console.error); } }, [dappId]); return { dappInfo, setDappInfo }; }packages/extension-polkagate/src/popup/authorize/Request.tsx (1)
Line range hint
42-47: Consider enhancing error handling and user feedback.The promise chains in
useEffectand action handlers only log errors to console. Consider:
- Adding proper error handling with user feedback
- Implementing loading states during async operations
Example improvement:
+ const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<string | null>(null); useEffect(() => { + setIsLoading(true); + setError(null); getAuthList() .then(({ list: authList }) => { const dappURL = extractBaseUrl(url); // ... existing code ... }) - .catch(console.error); + .catch((error: Error) => { + setError(t('Failed to load authorized accounts')); + console.error(error); + }) + .finally(() => setIsLoading(false)); }, [url]);Also applies to: 73-76
packages/extension-polkagate/src/components/AccountsTable.tsx (2)
30-41: Consider enhancing the sort function with secondary criteriaWhile the current implementation correctly sorts based on selection status, consider adding secondary sorting criteria (e.g., account name, address) when accounts have the same selection status. This would provide a more consistent and predictable ordering.
-const sortAccounts = (accountA: AccountJson, accountB: AccountJson, selectedList: string[]): number => { +const sortAccounts = (accountA: AccountJson, accountB: AccountJson, selectedList: string[]): number => { const isASelected = selectedList.includes(accountA.address); const isBSelected = selectedList.includes(accountB.address); if (!isASelected && isBSelected) { return -1; } else if (isASelected && !isBSelected) { return 1; } - return 0; + // Secondary sort by address when selection status is equal + return accountA.address.localeCompare(accountB.address); };
136-136: Consider memoizing child components for better performanceThe React.memo wrapper is good for preventing unnecessary re-renders of the entire component. Consider also memoizing the Identity component within the map function since it's being rendered for each account.
+const MemoizedIdentity = React.memo(Identity); // Then in the map function: -<Identity address={address} identiconSize={25} showShortAddress showSocial={false} style={{ fontSize: '14px' }} subIdOnly /> +<MemoizedIdentity address={address} identiconSize={25} showShortAddress showSocial={false} style={{ fontSize: '14px' }} subIdOnly />Also applies to: 161-161
packages/extension-polkagate/src/popup/home/YouHave.tsx (1)
94-149: Consider extracting balance display logic into separate componentsThe balance display section contains complex nested conditional rendering and multiple Stack components. This could be simplified for better maintainability and performance.
Consider breaking this into smaller components:
+ const BalanceDisplay = ({ youHave, currency }) => ( + <Stack alignItems='center' direction='row' justifyContent='space-between' + sx={{ flexWrap: 'wrap', mr: '15px', textAlign: 'start', width: '100%' }}> + {/* Balance amount JSX */} + </Stack> + ); + const AvailableBalance = ({ youHave }) => ( + <Stack alignItems='center' direction='row' spacing={1} + sx={{ ml: '5px', mt: '5px', textAlign: 'start', width: '100%' }}> + {/* Available balance JSX */} + </Stack> + ); {isHideNumbers ? <HiddenBalance /> : <> <BalanceDisplay youHave={youHave} currency={currency} /> <AvailableBalance youHave={youHave} /> </> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/extension-polkagate/src/components/AccountsTable.tsx(4 hunks)packages/extension-polkagate/src/partials/ConnectedDappIcon.tsx(3 hunks)packages/extension-polkagate/src/popup/authManagement/ManageAuthorizedAccounts.tsx(1 hunks)packages/extension-polkagate/src/popup/authManagement/ManageAuthorizedDapps.tsx(1 hunks)packages/extension-polkagate/src/popup/authManagement/index.tsx(4 hunks)packages/extension-polkagate/src/popup/authorize/Request.tsx(1 hunks)packages/extension-polkagate/src/popup/home/YouHave.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/popup/authManagement/ManageAuthorizedDapps.tsx
🔇 Additional comments (11)
packages/extension-polkagate/src/partials/ConnectedDappIcon.tsx (2)
10-10: LGTM: Clean hook import
The import is well-organized and properly grouped with other hook imports.
83-84: Consider organizing styles by category
The style properties would be more maintainable if grouped by category (positioning, dimensions, animation, etc.).
sx={{
+ // Positioning
position: 'absolute',
right: '44%',
top: 6,
zIndex: 10,
+ // Dimensions
width: '15px',
height: '15px',
+ // Appearance
borderRadius: '50%',
+ // Animation
transform: flip ? 'rotateY(180deg)' : 'rotateY(0deg)',
transition: 'transform 1s',
+ // Interaction
cursor: 'pointer',
}}Additionally, let's verify if the zIndex value is sufficient for your layout hierarchy.
Also applies to: 87-88
✅ Verification successful
z-index value of 10 is appropriate for the component's context
The z-index value of 10 is consistent with similar overlay components in the codebase, such as dropdowns, menus, and popups. The value is sufficient for the component's hierarchy as it matches other interactive elements that need to appear above the base content.
The style organization suggestion remains valid and would improve maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other z-index values in the codebase to ensure no conflicts
rg -i 'z-?index' --type tsx --type css
Length of output: 68
Script:
#!/bin/bash
# Search for z-index values in the codebase with ripgrep, using more generic file extensions
rg -i 'z-?index' -g '*.ts*' -g '*.css' -g '*.scss'
Length of output: 14327
packages/extension-polkagate/src/popup/authManagement/ManageAuthorizedAccounts.tsx (2)
56-56: LGTM: New prop aligns with PR objective
The addition of manageConnectedAccounts prop enables the sorting functionality for authorized accounts, which directly implements the PR's goal of sorting already authorized accounts to the bottom of the list.
60-60: LGTM: Minor style adjustment
The margin adjustment from 35px to 25px maintains consistent spacing while providing a more compact layout.
packages/extension-polkagate/src/popup/authManagement/index.tsx (1)
Line range hint 58-76: LGTM! Verify modal behavior with memoization.
The memoization is well-applied for the modal component. The implementation looks correct and follows React patterns.
Let's verify that no other components depend on FSMode's render cycle:
✅ Verification successful
Let me check how FSMode is used within the file to ensure there are no issues with the memoization.
Memoization implementation is safe and correctly applied
The FSMode component is only used once within a conditional render, receiving stable props from its parent. The memoization is properly implemented and won't cause any issues with the component's dependencies or render cycles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any components that might be affected by FSMode's memoization
rg -l "FSMode" packages/extension-polkagate/src/
Length of output: 112
Script:
#!/bin/bash
# Get the full content of the file to see FSMode usage
rg "FSMode" packages/extension-polkagate/src/popup/authManagement/index.tsx -A 5 -B 5
Length of output: 1037
packages/extension-polkagate/src/popup/authorize/Request.tsx (1)
89-90: LGTM! UI improvements for better account management.
The changes enhance the user experience by:
- Adding
manageConnectedAccountsprop to enable sorting of authorized accounts - Increasing the
maxHeightto provide better visibility of the accounts list
Let's verify the AccountsTable integration:
✅ Verification successful
The UI changes are properly integrated and working as expected
The verification confirms:
AccountsTablecomponent correctly implements both props:manageConnectedAccounts(optional boolean) for sorting functionalitymaxHeight(optional string|number) with a default of '112px'
- The sorting logic is implemented using a ref-based memoization for performance
- The increased maxHeight (280px/300px) is within reasonable bounds compared to other table components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AccountsTable component handles the new prop correctly
# Check AccountsTable implementation
ast-grep --pattern 'interface Props {
$$$
manageConnectedAccounts?: boolean;
$$$
}'
# Check for sorting logic
rg -A 10 'manageConnectedAccounts.*sort'
Length of output: 2313
Script:
#!/bin/bash
# Let's verify the sorting logic implementation
ast-grep --pattern 'function sortAccounts($$$) {
$$$
}'
# Also check if there are any potential issues with the maxHeight prop
ast-grep --pattern 'interface Props {
$$$
maxHeight?: string | number;
$$$
}'
rg -A 5 'maxHeight.*style'
Length of output: 8031
packages/extension-polkagate/src/components/AccountsTable.tsx (2)
6-6: LGTM: Type changes are well-defined
The new AccountJson import and Props interface update are properly typed and align with the component's enhanced functionality.
Also applies to: 27-27
48-68: 🛠️ Refactor suggestion
Remove commented-out code and verify sorting behavior
The sorting implementation looks good, but there's commented-out sorting logic on line 67 that should be removed to maintain clean code.
Also, the current implementation only sorts once and caches the result. This means if selectedAccounts changes, the sort order won't update. Is this the intended behavior?
Consider this alternative implementation if you need dynamic sorting:
- const sortedAccountsRef = useRef<AccountJson[] | null>(null);
-
const accountsToShow = useMemo(() => {
const filtered = [...accounts].filter(({ isExternal, isHardware, isHidden, isQR }) =>
(accountTypeFilter?.includes('Watch-Only') && !isExternal) ||
(accountTypeFilter?.includes('Hardware') && !isHardware) ||
(accountTypeFilter?.includes('QR') && !isQR) ||
!isHidden
);
- // Only sort accounts when:
- // 1. We're in manage authorized accounts mode (manageConnectedAccounts is true)
- // 2. The accounts haven't been sorted yet (sortedAccountsRef.current is null)
- // 3. There are some selected accounts (selectedAccounts.length !== 0)
- if (manageConnectedAccounts && !sortedAccountsRef.current && selectedAccounts.length !== 0) {
- sortedAccountsRef.current = [...filtered].sort((a, b) => sortAccounts(a, b, selectedAccounts));
- }
+ return manageConnectedAccounts && selectedAccounts.length !== 0
+ ? [...filtered].sort((a, b) => sortAccounts(a, b, selectedAccounts))
+ : filtered;
- return filtered; // .sort((a, b) => sortAccounts(a, b, selectedAccounts))
}, [accountTypeFilter, accounts, manageConnectedAccounts, selectedAccounts]);packages/extension-polkagate/src/popup/home/YouHave.tsx (3)
22-22: LGTM: Import statement is properly placed
The new import for ConnectedDappIcon follows the project's import organization pattern.
88-93: LGTM: Grid layout structure is well-organized
The Grid container structure with flex-start alignment provides a clean layout for the balance information. The Typography styling follows Material-UI best practices.
160-168: Verify layout consistency across different screen sizes
The absolute positioning of HideBalance component might cause layout issues on different screen sizes or when the parent container's dimensions change.
✅ Verification successful
Based on the search results, I can see that absolute positioning is commonly used throughout the codebase, particularly in similar UI components. The HideBalance component's positioning in YouHave.tsx follows a consistent pattern with other UI elements that need to be precisely placed relative to their container.
Let's verify the parent container's layout to ensure it provides proper containment:
Let's try a different approach to check the component's structure:
Based on the code context, I can now provide a final response:
Absolute positioning is appropriate in this context
The HideBalance component's absolute positioning is intentionally used within a properly structured container. The parent Grid component provides a relative positioning context, and the absolute positioning is used consistently with other UI elements in the same component (like the menu button and logo). This pattern ensures:
- Precise alignment of UI controls
- Consistent layout across the application
- Proper containment within the parent container's boundaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive layout handling in similar components
rg -g '*.tsx' -g '*.jsx' 'position: .absolute.' packages/extension-polkagate/src/
Length of output: 30109
Script:
#!/bin/bash
# Check the parent container's styling and layout
ast-grep --pattern 'const YouHave = $$$' packages/extension-polkagate/src/popup/home/YouHave.tsx
Length of output: 98
Script:
#!/bin/bash
# Check the component's structure and container layout
rg -A 10 -B 10 'position: .absolute.' packages/extension-polkagate/src/popup/home/YouHave.tsx
Length of output: 2078
| size={20} | ||
| /> | ||
| </Grid> | ||
| <ConnectedDappIcon /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary and consider explicit positioning
The ConnectedDappIcon component might benefit from:
- Error boundary wrapper to handle potential animation failures
- Explicit positioning props to ensure consistent layout
Consider wrapping it with error boundary and adding positioning:
- <ConnectedDappIcon />
+ <ErrorBoundary fallback={<FallbackComponent />}>
+ <ConnectedDappIcon
+ sx={{ position: 'absolute', bottom: '10px', right: '10px' }}
+ />
+ </ErrorBoundary>Committable suggestion skipped: line range outside the PR's diff.
# [0.31.0](v0.30.2...v0.31.0) (2024-11-17) ### Features * sort the already authorized accounts to the bottom of the list to easier auth the unauthorized accounts ([#1662](#1662)) ([8f0fc1d](8f0fc1d))
Summary by CodeRabbit
Release Notes
New Features
manageConnectedAccountsto enhance account management functionality.Improvements
Performance Enhancements