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-2011] collections hotsync #162

Merged
merged 2 commits into from
Sep 25, 2024
Merged

[PLA-2011] collections hotsync #162

merged 2 commits into from
Sep 25, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Sep 24, 2024

PR Type

enhancement, bug fix


Description

  • Added a hotSync parameter to the trackCollection method and its associated API call, allowing for conditional synchronization based on tenant configuration.
  • Enhanced the TrackCollectionModal component by adding a loading state to the 'Track' button and adjusting the lifecycle hook to reset states on mount.
  • Improved error handling in the trackCollection function by changing the snackbar message type to error on failure and ensuring the modal closes after an attempt.
  • Updated the GraphQL mutation to include the hotSync parameter, enabling dynamic synchronization control.

Changes walkthrough 📝

Relevant files
Enhancement
TrackCollectionModal.vue
Add loading state and lifecycle hook adjustment in
TrackCollectionModal

resources/js/components/TrackCollectionModal.vue

  • Added loading state to the 'Track' button.
  • Changed lifecycle hook from onUnmounted to onMounted.
  • Reset collectionId and loading state on modal mount.
  • +7/-4     
    Collections.vue
    Enhance trackCollection with hotSync and error handling   

    resources/js/components/pages/Collections.vue

  • Added hotSync parameter to trackCollection API call.
  • Changed snackbar message type from info to error on failure.
  • Ensured modal closes after tracking attempt.
  • +5/-2     
    collection.ts
    Add hotSync parameter to trackCollection API method           

    resources/js/api/collection.ts

  • Added hotSync parameter to trackCollection method.
  • Updated API request to include hotSync variable.
  • +2/-1     
    TrackCollection.ts
    Update GraphQL mutation to support hotSync parameter         

    resources/js/graphql/mutation/TrackCollection.ts

  • Added hotSync parameter to GraphQL mutation.
  • Updated mutation to use dynamic hotSync value.
  • +2/-2     

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

    Copy link

    PR Reviewer Guide 🔍

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

    State Reset Logic
    The onMounted lifecycle hook resets collectionId and loading states every time the component is mounted. This could lead to unexpected behavior if the component is reused without being destroyed. Consider if this reset is necessary or if it should be handled differently.

    Error Handling
    The error handling in trackCollection method changes the snackbar message type to error, which is good. However, ensure that all possible error scenarios are handled adequately, especially with the new hotSync parameter involved.

    API Method Update
    The trackCollection method in the API now includes a hotSync parameter. Ensure that all callers of this method are updated to handle this new parameter correctly.

    Copy link

    github-actions bot commented Sep 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add a finally block to reset loading state

    It's recommended to add a finally block to reset the loading state to false after
    the confirm method completes. This ensures that the button is re-enabled even if the
    operation fails.

    resources/js/components/TrackCollectionModal.vue [43-46]

     const confirm = async () => {
         loading.value = true;
    -    emit('confirm', collectionId.value);
    +    try {
    +        emit('confirm', collectionId.value);
    +    } finally {
    +        loading.value = false;
    +    }
     };
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly adds a finally block to ensure the loading state is reset, improving the reliability and maintainability of the code by ensuring the button is re-enabled regardless of the operation's success or failure.

    9
    Error handling
    Add error handling for the getCollections call

    Consider adding error handling for the getCollections() call within the
    trackCollection method to manage potential failures gracefully.

    resources/js/components/pages/Collections.vue [431-432]

     await CollectionApi.trackCollection(collectionId, hotSync);
    -await getCollections();
    +try {
    +    await getCollections();
    +} catch (error) {
    +    console.error('Failed to fetch collections:', error);
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the getCollections call is a good practice to prevent unhandled promise rejections and to provide feedback in case of failure, enhancing the robustness of the application.

    8
    Validate collectionId before parsing to integer

    Ensure that parseInt is safely used by checking if collectionId is a valid number
    before parsing, to avoid runtime errors.

    resources/js/api/collection.ts [165]

    -chainIds: [parseInt(collectionId)],
    +chainIds: [!isNaN(parseInt(collectionId)) ? parseInt(collectionId) : throw new Error("Invalid collection ID")],
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by ensuring that collectionId is a valid number before parsing, which can prevent potential runtime errors, although the use of throw in a ternary is unconventional and could be improved.

    7
    Documentation
    Add a description for the hotSync parameter

    Add a description for the hotSync parameter in the GraphQL mutation to improve code
    documentation and clarity.

    resources/js/graphql/mutation/TrackCollection.ts [1-3]

     export default `mutation addToTracked($type: ModelType! = COLLECTION, $chainIds:[BigInt!]!, $hotSync: Boolean = false) {
    +    # hotSync: If true, performs a hot synchronization; otherwise, a regular sync.
         AddToTracked(type: $type, chainIds: $chainIds, hotSync: $hotSync)
     }`;
     
    Suggestion importance[1-10]: 5

    Why: Adding a description for the hotSync parameter improves code documentation and clarity, which is beneficial for maintainability, although it does not address a critical issue.

    5

    @zlayine zlayine merged commit b5a13f2 into master Sep 25, 2024
    3 checks passed
    @zlayine zlayine deleted the bugfix/pla-2011/hotsync branch September 25, 2024 10:03
    zlayine added a commit that referenced this pull request Sep 25, 2024
    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