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

Add a source network and destination network selector #792

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

marc-aurele-besner
Copy link
Member

@marc-aurele-besner marc-aurele-besner commented Aug 3, 2024

User description

Add a source network and destination network selector

Screenshot

Swap/TransferSection
SendTokenSidekick


PR Type

Enhancement


Description

  • Added a new NetworkSelector component to allow users to select source and destination networks.
  • Introduced a NetworkSource enum to define network types (CONSENSUS and DOMAIN).
  • Updated the SendTokenFormValues interface to include sender, sourceNetwork, sourceDomainId, destinationNetwork, and destinationDomainId fields.
  • Integrated the network selection functionality into the ActionsModal form, allowing users to specify the source and destination networks when sending tokens.

Changes walkthrough 📝

Relevant files
Enhancement
ActionsModal.tsx
Add network selection functionality to ActionsModal           

explorer/src/components/WalletSideKick/ActionsModal.tsx

  • Added NetworkSelector component for selecting source and destination
    networks.
  • Introduced NetworkSource enum for network types.
  • Updated SendTokenFormValues to include network-related fields.
  • Integrated network selection into the ActionsModal form.
  • +205/-3 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    netlify bot commented Aug 3, 2024

    Deploy Preview for dev-astral ready!

    Name Link
    🔨 Latest commit 8462738
    🔍 Latest deploy log https://app.netlify.com/sites/dev-astral/deploys/66b2207b3d4a360008fcc27f
    😎 Deploy Preview https://deploy-preview-792--dev-astral.netlify.app
    📱 Preview on mobile
    Toggle QR Code...

    QR Code

    Use your smartphone camera to open QR code link.

    To edit notification comments on pull requests, go to your Netlify site configuration.

    Copy link

    github-actions bot commented Aug 3, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    The NetworkSelector component logic is duplicated for both source and destination network selection. Consider refactoring to use a single component instance or abstracting common functionality to reduce redundancy and improve maintainability.

    Possible Bug
    The setNetwork and setDomainId functions are called with potentially undefined values from the network['domains'] array without null checks. This could lead to runtime errors if the array is empty or the expected properties are missing.

    UI Consistency
    The CSS classes applied in the NetworkSelector component vary significantly between different states (e.g., selected vs. non-selected). Ensure consistent styling to maintain a uniform user interface.

    Hardcoded Values
    The initial values for sourceNetwork and destinationNetwork are hardcoded to NetworkSource.CONSENSUS. Consider making these configurable or ensuring they are set based on user preferences or previous selections to enhance flexibility.

    Copy link

    github-actions bot commented Aug 3, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Use unique names for each FieldArray to avoid state management issues

    The FieldArray component's name attribute is set to 'dischargeNorms' for multiple
    different fields, which could lead to unexpected behavior as it might mix up the
    state management for different fields. Each FieldArray should have a unique name
    reflecting its purpose.

    explorer/src/components/WalletSideKick/ActionsModal.tsx [504-591]

     <FieldArray
    -  name='dischargeNorms'
    +  name='sourceNetworkFields'
    +<FieldArray
    +  name='destinationNetworkFields'
     
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a critical issue by ensuring that each FieldArray has a unique name, which is essential for correct state management and avoiding unexpected behavior.

    10
    Add safe navigation to prevent potential runtime errors when network['domains'] is undefined

    Consider using a more robust error handling mechanism for the network and domain ID
    selections. Currently, the code does not handle cases where network['domains'] might
    be undefined or empty, which could lead to runtime errors.

    explorer/src/components/WalletSideKick/ActionsModal.tsx [120]

    -{network['domains'].map((domain, index) => (
    +{network['domains']?.map((domain, index) => (
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by adding safe navigation, which is a crucial improvement for stability and robustness.

    9
    Possible issue
    Add dependencies to useMemo to ensure it updates when networks changes

    The useMemo hook is used to memoize the first network but does not specify
    dependencies. This could lead to stale data being used if the networks array
    changes. Consider adding networks as a dependency to the useMemo hook.

    explorer/src/components/WalletSideKick/ActionsModal.tsx [466]

    -const network = useMemo(() => networks[0], [])
    +const network = useMemo(() => networks[0], [networks])
     
    Suggestion importance[1-10]: 8

    Why: Adding dependencies to the useMemo hook ensures that the memoized value updates correctly when the networks array changes, preventing potential bugs related to stale data.

    8
    Replace hardcoded network source values with dynamic user-selected values

    Replace the hardcoded 'NetworkSource.CONSENSUS' and 'NetworkSource.DOMAIN' in the
    onClick handlers with dynamic values based on the actual selected network. This will
    ensure that the correct network source is set based on user selection rather than
    being hardcoded, which could lead to incorrect behavior if the network context
    changes.

    explorer/src/components/WalletSideKick/ActionsModal.tsx [103-131]

    -setNetwork(NetworkSource.CONSENSUS)
    -setNetwork(NetworkSource.DOMAIN)
    +setNetwork(e.target.value)
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves flexibility and correctness by using dynamic values instead of hardcoded ones. However, the exact implementation details are not fully clear from the suggestion, and it might require additional context to implement correctly.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant