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.

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

[PLA-2020] wallet list sign #165

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Oct 7, 2024

PR Type

enhancement, bug_fix


Description

  • Enhanced account management by introducing a computed property walletAccounts to filter enabled accounts and updating components to use this property.
  • Added a new component WalletAppItem for managing wallet accounts, including enabling and disabling accounts.
  • Fixed media array initialization in the collection creation process to prevent errors when image URLs are not present.
  • Improved connection store by adding disabledAccounts state and methods to manage account enable/disable status.
  • Ensured wallet accounts default to an empty array to prevent undefined errors.

Changes walkthrough 📝

Relevant files
Enhancement
SignTransaction.vue
Update account handling with computed wallet accounts       

resources/js/components/SignTransaction.vue

  • Replaced connectionStore.accounts with walletAccounts for account
    iteration.
  • Added a computed property walletAccounts to filter enabled accounts.
  • Updated condition to check for empty walletAccounts.
  • +5/-3     
    SettingsWalletApp.vue
    Refactor wallet app settings with loading and item component

    resources/js/components/pages/SettingsWalletApp.vue

  • Replaced account display with WalletAppItem component.
  • Added loading indicator for account fetching.
  • Removed clipboard copy functionality.
  • +24/-29 
    WalletAppItem.vue
    Add WalletAppItem component for account management             

    resources/js/components/pages/WalletAppItem.vue

  • Created new component for displaying wallet account items.
  • Added functionality to enable or disable wallet accounts.
  • +43/-0   
    connection.ts
    Enhance connection store with account enable/disable functionality

    resources/js/store/connection.ts

  • Added disabledAccounts to store state and persistence.
  • Implemented methods to get and toggle wallet account enabled status.
  • +30/-1   
    types.interface.ts
    Update ConnectionState interface with disabled accounts   

    resources/js/types/types.interface.ts

    • Added disabledAccounts to ConnectionState interface.
    +1/-0     
    Bug fix
    WalletConnectButton.vue
    Ensure wallet accounts default to empty array                       

    resources/js/components/WalletConnectButton.vue

  • Added default empty array for walletAccounts to prevent undefined
    errors.
  • +1/-1     
    CreateCollection.vue
    Fix media array initialization in collection creation       

    resources/js/components/pages/create/CreateCollection.vue

  • Fixed media array initialization to prevent errors when image URLs are
    not present.
  • +5/-4     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Oct 7, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The new code introduces a computed property walletAccounts which seems to duplicate functionality with existing account management in the connection store. Consider consolidating account management to ensure consistency and reduce maintenance overhead.

    Error Handling
    The method enableWalletAccount in the connection store modifies disabledAccounts directly without proper checks or error handling which might lead to inconsistent state or errors during runtime.

    UI Responsiveness
    The loading state management for accounts in the wallet app settings page might cause UI responsiveness issues or race conditions due to the lack of cancellation or handling of concurrent executions.

    Copy link

    github-actions bot commented Oct 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Implement error handling for account fetching to enhance reliability

    Add error handling for the connectionStore.getAccounts() method within the watch
    function to manage potential failures gracefully.

    resources/js/components/pages/SettingsWalletApp.vue [91]

    -await connectionStore.getAccounts();
    +try {
    +    await connectionStore.getAccounts();
    +} catch (error) {
    +    console.error('Failed to fetch accounts:', error);
    +}
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the connectionStore.getAccounts() method is a valuable enhancement that improves the robustness of the code by ensuring that failures in fetching accounts are managed gracefully.

    8
    Implement error logging in the getWalletAccounts method for better debugging

    Add error handling in the getWalletAccounts method to catch and log potential errors
    during account processing.

    resources/js/store/connection.ts [234-244]

     try {
         accounts = accounts?.map((account) => {
             if (this.disabledAccounts?.find((disabled) => disabled === account.address)) {
                 return { ...account, enabled: false };
             }
             return { ...account, enabled: true };
         });
    -} catch {
    -    //
    +} catch (error) {
    +    console.error('Error processing wallet accounts:', error);
     }
    Suggestion importance[1-10]: 6

    Why: Adding error logging in the getWalletAccounts method is a useful enhancement for debugging purposes, as it provides visibility into any issues that may arise during account processing.

    6
    Possible bug
    Add null check for walletAccounts to prevent runtime errors

    Ensure that the walletAccounts array is checked for null or undefined before
    checking its length to avoid potential runtime errors.

    resources/js/components/SignTransaction.vue [54]

    -<div v-if="!walletAccounts.length" class="text-center">
    +<div v-if="walletAccounts && !walletAccounts.length" class="text-center">
    Suggestion importance[1-10]: 7

    Why: Adding a null check for walletAccounts before checking its length is a good practice to prevent potential runtime errors, especially if there's any chance that walletAccounts could be null or undefined.

    7

    @zlayine zlayine merged commit 3dbbfa7 into master Oct 8, 2024
    3 checks passed
    @zlayine zlayine deleted the feature/pla-2020/wallet-list-sign branch October 8, 2024 12:31
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants