Feat: Reorganize Wallet PopUp #945
Conversation
Added the function sortConnectors and applied logic to display available connectors first, followed by unavailable ones.
|
@Villarley is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a new utility function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
components/UI/navbar.tsxOops! Something went wrong! :( ESLint: 9.14.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 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: 2
🧹 Outside diff range and nitpick comments (1)
utils/sorts.ts (1)
3-6: Add input validation for robustnessWhile the function signature is well-typed, it would be more robust with input validation.
Consider adding validation:
export const sortConnectors = (connectors: Connector[]): Connector[] => { + if (!connectors?.length) { + return []; + } const available: Connector[] = []; const notAvailable: Connector[] = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/UI/navbar.tsx(3 hunks)utils/sorts.ts(1 hunks)
🔇 Additional comments (5)
utils/sorts.ts (2)
1-2: LGTM: Clean import statement
The import is correctly typed and aligns with the starknetkit integration mentioned in the PR objectives.
1-24: Verify integration with Navbar component
The implementation aligns well with the PR objectives. Let's verify its integration with the Navbar component.
✅ Verification successful
Let me gather more context about the integration by checking the complete implementation of the navbar component.
Let me check the source of availableConnectors to complete the verification.
Integration verified successfully
The sortConnectors function is properly integrated in the Navbar component:
- It's correctly imported and used to sort the
availableConnectorsfrom the provider - The sorted connectors are passed to
useStarknetkitConnectModalhook - The implementation maintains the wallet connection functionality while adding the desired sorting behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of sortConnectors in the Navbar component
# Search for usage of sortConnectors in the codebase
echo "Checking usage of sortConnectors:"
rg -l "sortConnectors"
# Look for Navbar component implementation
echo "\nChecking Navbar implementation:"
rg -A 5 "useStarknetkitConnectModal" "components/"
Length of output: 1109
Script:
#!/bin/bash
# Get the complete implementation of navbar.tsx to verify sortConnectors integration
echo "Complete navbar implementation:"
cat components/UI/navbar.tsx
# Check if sortedConnectors is properly initialized
echo "\nChecking sortedConnectors initialization:"
rg -B 5 -A 2 "sortedConnectors" components/UI/navbar.tsx
Length of output: 12490
Script:
#!/bin/bash
# Check the implementation of availableConnectors
echo "Checking availableConnectors source:"
rg -B 2 -A 2 "availableConnectors" "app/"
# Check if there are any other files importing availableConnectors
echo "\nChecking availableConnectors imports:"
rg -l "availableConnectors"
Length of output: 1454
components/UI/navbar.tsx (3)
29-29: LGTM: Clean import statement
The import of sortConnectors from @utils/sorts is properly placed with other utility imports.
57-60: Verify connector sorting behavior
The sorting logic needs verification to ensure it properly handles all connector states and edge cases.
Let's verify the connector sorting implementation:
✅ Verification successful
Let me check the implementation of the sorting function to verify its behavior.
Sorting implementation is robust with proper handling of connector states
The sortConnectors implementation is well-structured and handles the key scenarios:
- Separates available and unavailable connectors
- Places all available connectors first
- Sorts unavailable connectors in a specific order (okxwallet, bitkeep)
- Handles edge cases by using array methods that work with empty arrays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and analyze the sortConnectors implementation
ast-grep --pattern 'export function sortConnectors($$$)'
# Check for any existing tests of the sorting logic
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx | xargs rg "sortConnectors"
Length of output: 385
Script:
#!/bin/bash
# Get the implementation of sortConnectors
cat utils/sorts.ts
Length of output: 647
57-60: 🛠️ Refactor suggestion
Verify type safety of connector casting
The implementation looks functionally correct, but there are two concerns:
- The type casting
as anysuggests potential type safety issues that should be addressed. - The sorting is performed on component render which could be optimized.
Consider these improvements:
- const sortedConnectors = sortConnectors(availableConnectors);
+ const sortedConnectors = useMemo(
+ () => sortConnectors(availableConnectors),
+ [availableConnectors]
+ );
const { starknetkitConnectModal } = useStarknetkitConnectModal({
- connectors: sortedConnectors as any,
+ connectors: sortedConnectors,
});Let's verify the connector types:
✅ Verification successful
Let me gather more information about the connector types and their usage.
Let me try a different approach to gather information about the types and implementations.
Let me try one final time without specifying the file type.
Based on the gathered information, I can now provide the final response:
Type casting is necessary, performance optimization is valid
The as any type casting in useStarknetkitConnectModal is actually necessary because:
- The
sortConnectorsfunction correctly returnsConnector[]from starknetkit - The hook appears to have slightly different type expectations from the starknet-react/core Connector type
The performance optimization suggestion using useMemo is valid since sortConnectors performs array operations on every render.
utils/sorts.ts: ImplementssortConnectorswith proper typing- Multiple files show consistent use of type casting between connector types
- The type casting pattern is used consistently across the codebase in similar scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the Connector type definitions in the codebase
rg -A 3 "type.*Connector.*=|interface.*Connector.*{"
# Check usage of type casting in connector-related code
rg "as any.*Connector|as Connector"
Length of output: 688
Script:
#!/bin/bash
# Check type definitions with fixed regex
rg -A 3 "type.*Connector|interface.*Connector"
# Check sortConnectors implementation
ast-grep --pattern 'function sortConnectors'
# Check starknetkitConnectModal hook type
rg -A 3 "useStarknetkitConnectModal.*="
Length of output: 137
Script:
#!/bin/bash
# Search for connector-related imports and type definitions
rg -A 2 "import.*Connector|type.*Connector" --type typescript
# Find sortConnectors implementation with different pattern
rg -A 5 "sortConnectors.*=" --type typescript
# Look for starknetkitConnectModal related code
rg -A 3 "starknetkitConnectModal" --type typescript
Length of output: 268
Script:
#!/bin/bash
# Search for connector-related imports and type definitions
rg -A 2 "import.*Connector|type.*Connector"
# Find sortConnectors implementation
rg -A 5 "const sortConnectors|function sortConnectors"
# Look for starknetkitConnectModal related code and types
rg -A 3 "starknetkitConnectModal|useStarknetkitConnectModal"
# Check the specific file content
cat components/UI/navbar.tsx
Length of output: 15977
utils/sorts.ts
Outdated
| return [ | ||
| ...available, | ||
| // Reorganized not available connectors | ||
| ...notAvailable.sort((a, b) => { | ||
| const order = ["okxwallet", "bitkeep"]; // desired order | ||
| return order.indexOf(a.id) - order.indexOf(b.id); | ||
| }), | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve sorting configuration and handle unknown connectors
The current implementation has two potential issues:
- The order array is hardcoded and not configurable
- Connectors not in the order array will be sorted incorrectly (both will return -1 from indexOf)
Consider this improved implementation:
+const DEFAULT_ORDER = ["okxwallet", "bitkeep"];
+
export const sortConnectors = (connectors: Connector[]): Connector[] => {
const available: Connector[] = [];
const notAvailable: Connector[] = [];
+ const order = DEFAULT_ORDER;
// Rest of the code...
return [
...available,
...notAvailable.sort((a, b) => {
- const order = ["okxwallet", "bitkeep"]; // desired order
- return order.indexOf(a.id) - order.indexOf(b.id);
+ const indexA = order.indexOf(a.id);
+ const indexB = order.indexOf(b.id);
+ // If neither connector is in the order array, maintain their relative position
+ if (indexA === -1 && indexB === -1) return 0;
+ // If only one connector is in the order array, prioritize it
+ if (indexA === -1) return 1;
+ if (indexB === -1) return -1;
+ // Both connectors are in the order array, sort by their index
+ return indexA - indexB;
}),
];
};📝 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.
| return [ | |
| ...available, | |
| // Reorganized not available connectors | |
| ...notAvailable.sort((a, b) => { | |
| const order = ["okxwallet", "bitkeep"]; // desired order | |
| return order.indexOf(a.id) - order.indexOf(b.id); | |
| }), | |
| ]; | |
| }; | |
| const DEFAULT_ORDER = ["okxwallet", "bitkeep"]; | |
| return [ | |
| ...available, | |
| ...notAvailable.sort((a, b) => { | |
| const indexA = order.indexOf(a.id); | |
| const indexB = order.indexOf(b.id); | |
| // If neither connector is in the order array, maintain their relative position | |
| if (indexA === -1 && indexB === -1) return 0; | |
| // If only one connector is in the order array, prioritize it | |
| if (indexA === -1) return 1; | |
| if (indexB === -1) return -1; | |
| // Both connectors are in the order array, sort by their index | |
| return indexA - indexB; | |
| }), | |
| ]; | |
| }; |
utils/sorts.ts
Outdated
| @@ -0,0 +1,24 @@ | |||
| import { Connector } from "starknetkit"; | |||
There was a problem hiding this comment.
rename the file connectors.ts, we'll reuse it later for connectors utils
utils/sorts.ts
Outdated
| return order.indexOf(a.id) - order.indexOf(b.id); | ||
| }), | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
you need to create a connector.test.js function to test your util and make sur it covers all hedge cases here :)
- Updated function import at the navbar file. - Created connectors.test.js for testing the util function. - Renamed the file to "connectors.ts"
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
utils/connectors.ts (2)
3-6: Add JSDoc documentation for better maintainabilityConsider adding JSDoc documentation to describe the function's purpose, parameters, and return value.
+/** + * Sorts wallet connectors by availability, placing available connectors first, + * followed by unavailable ones in a specific order. + * @param connectors - Array of wallet connectors to sort + * @returns Sorted array of connectors with available ones first + */ export const sortConnectors = (connectors: Connector[]): Connector[] => {
7-14: Consider using filter for more functional approach and add input validationThe current implementation could be improved for robustness and maintainability.
- // Sort connectors - connectors.forEach((connector) => { - if (connector.available()) { - available.push(connector); - } else { - notAvailable.push(connector); - } - }); + if (!Array.isArray(connectors)) { + throw new Error('Input must be an array of connectors'); + } + + // Sort connectors into available and unavailable + const available = connectors.filter(connector => connector.available()); + const notAvailable = connectors.filter(connector => !connector.available());tests/utils/connectors.test.js (1)
13-58: Consider adding tests for edge casesWhile the current test coverage is good, consider adding the following test cases to improve robustness:
- Verify that the original array remains unmodified (pure function)
- Handle null/undefined input
- Validate connector object structure
- Test order preservation within same-availability groups
Here's a suggested implementation:
it("should not modify the original array", () => { const connectors = [ new MockConnector("okx", "Okx Wallet", false), new MockConnector("braavos", "Braavos", true), ]; const originalConnectors = [...connectors]; sortConnectors(connectors); expect(connectors).toEqual(originalConnectors); }); it("should handle invalid input gracefully", () => { expect(sortConnectors(null)).toEqual([]); expect(sortConnectors(undefined)).toEqual([]); }); it("should validate connector structure", () => { const invalidConnectors = [ { id: "test", name: "Test" }, // missing available method ]; expect(() => sortConnectors(invalidConnectors)).toThrow(); }); it("should preserve order within same-availability group", () => { const connectors = [ new MockConnector("wallet1", "Wallet 1", true), new MockConnector("wallet2", "Wallet 2", true), ]; const sortedConnectors = sortConnectors([...connectors]); expect(sortedConnectors[0].id).toBe(connectors[0].id); expect(sortedConnectors[1].id).toBe(connectors[1].id); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/UI/navbar.tsx(3 hunks)tests/utils/connectors.test.js(1 hunks)utils/connectors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/UI/navbar.tsx
🔇 Additional comments (4)
utils/connectors.ts (2)
1-24: Verify integration with Navbar and test coverage
Since this is a critical feature affecting wallet connectivity, let's verify:
- Integration with Navbar component
- Test coverage for all scenarios
#!/bin/bash
# Check Navbar integration
echo "Checking Navbar integration:"
rg --type tsx "sortConnectors.*availableConnectors" -A 5
# Check test coverage
echo -e "\nChecking test coverage:"
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | grep -i "connector"1-1: Verify starknetkit version compatibility
The PR objectives mention verification of compatibility between @starknet-react/core and starknetkit. Let's ensure we're using a compatible version.
tests/utils/connectors.test.js (2)
1-11: LGTM! Clean mock implementation
The MockConnector class effectively simulates the connector interface with the necessary properties and methods.
13-28: LGTM! Well-structured test cases
The test suite provides good coverage of the main sorting functionality with clear test descriptions and realistic test data.
| return [ | ||
| ...available, | ||
| // Reorganized not available connectors | ||
| ...notAvailable.sort((a, b) => { | ||
| const order = ["okxwallet", "bitkeep"]; // desired order | ||
| return order.indexOf(a.id) - order.indexOf(b.id); | ||
| }), | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Improve robustness of connector ordering logic
The current implementation has several potential issues:
- The order array is hardcoded and should be configurable
- The sorting logic doesn't handle connectors that aren't in the order array
- No type safety for connector IDs
Consider this improved implementation:
+const WALLET_ORDER = ["okxwallet", "bitkeep"] as const;
+type KnownWalletId = typeof WALLET_ORDER[number];
+
export const sortConnectors = (connectors: Connector[]): Connector[] => {
// ... previous code ...
return [
...available,
- // Reorganized not available connectors
...notAvailable.sort((a, b) => {
- const order = ["okxwallet", "bitkeep"]; // desired order
- return order.indexOf(a.id) - order.indexOf(b.id);
+ const aIndex = WALLET_ORDER.indexOf(a.id as KnownWalletId);
+ const bIndex = WALLET_ORDER.indexOf(b.id as KnownWalletId);
+
+ // If neither wallet is in the order array, maintain their relative position
+ if (aIndex === -1 && bIndex === -1) return 0;
+ // If only one wallet is in the order array, it should come first
+ if (aIndex === -1) return 1;
+ if (bIndex === -1) return -1;
+ // Both wallets are in the order array, sort by their index
+ return aIndex - bIndex;
}),
];
};📝 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.
| return [ | |
| ...available, | |
| // Reorganized not available connectors | |
| ...notAvailable.sort((a, b) => { | |
| const order = ["okxwallet", "bitkeep"]; // desired order | |
| return order.indexOf(a.id) - order.indexOf(b.id); | |
| }), | |
| ]; | |
| }; | |
| const WALLET_ORDER = ["okxwallet", "bitkeep"] as const; | |
| type KnownWalletId = typeof WALLET_ORDER[number]; | |
| return [ | |
| ...available, | |
| ...notAvailable.sort((a, b) => { | |
| const aIndex = WALLET_ORDER.indexOf(a.id as KnownWalletId); | |
| const bIndex = WALLET_ORDER.indexOf(b.id as KnownWalletId); | |
| // If neither wallet is in the order array, maintain their relative position | |
| if (aIndex === -1 && bIndex === -1) return 0; | |
| // If only one wallet is in the order array, it should come first | |
| if (aIndex === -1) return 1; | |
| if (bIndex === -1) return -1; | |
| // Both wallets are in the order array, sort by their index | |
| return aIndex - bIndex; | |
| }), | |
| ]; | |
| }; |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Added the function sortConnectors and applied logic to display available connectors first, followed by unavailable ones.
Pull Request type
Resolves: #932
Summary of Changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests