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

Change filters to query #106

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Change filters to query #106

merged 2 commits into from
Jun 13, 2024

Conversation

jamalsoueidan
Copy link
Owner

@jamalsoueidan jamalsoueidan commented Jun 13, 2024

PR Type

Enhancement, Bug fix


Description

  • Refactored query syntax in artist_.$username.treatment.$productHandle.tsx to use tag_not instead of -tag.
  • Added a new loader function and pagination handling in treatments.$productHandle._index.tsx for fetching and displaying treatment collection data.
  • Moved pagination handling to an Outlet component in treatments.$productHandle.tsx.
  • Updated storefrontapi.generated.d.ts to reflect changes in the treatment collection query, replacing handle with query and removing filters.

Changes walkthrough 📝

Relevant files
Enhancement
artist_.$username.treatment.$productHandle.tsx
Refactor query syntax in artist treatment loader                 

app/routes/artist_.$username.treatment.$productHandle.tsx

  • Refactored query syntax to use tag_not instead of -tag.
+1/-1     
treatments.$productHandle._index.tsx
Add loader and pagination for treatment collection             

app/routes/treatments.$productHandle._index.tsx

  • Added new loader function to fetch treatment collection data.
  • Implemented pagination handling within the component.
  • Added various UI components for displaying treatment products.
  • +602/-0 
    treatments.$productHandle.tsx
    Refactor to move pagination handling to Outlet component 

    app/routes/treatments.$productHandle.tsx

  • Removed pagination handling and related imports.
  • Moved pagination handling to an Outlet component.
  • +5/-526 
    Bug fix
    storefrontapi.generated.d.ts
    Update storefront API types for treatment collection query

    storefrontapi.generated.d.ts

  • Updated TreatmentCollectionQueryVariables to use query instead of
    handle.
  • Removed filters from TreatmentCollectionQueryVariables.
  • +98/-107

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

     - Update query syntax from `-tag:"hide_from_combine"` to `tag_not:"hide-from-combine"` in artist treatment loader.
     - Remove PaginationContent and related code from treatments route, moving pagination handling to an Outlet component.
     - Remove unused imports and fragments related to pagination and user collection.
    Copy link
    Contributor

    shopify bot commented Jun 13, 2024

    Oxygen deployed a preview of your change-filters-to-query branch. Details:

    Storefront Status Preview link Deployment details Last update (UTC)
    BySisters ✅ Successful (Logs) Preview deployment Inspect deployment June 13, 2024 9:58 PM

    Learn more about Hydrogen's GitHub integration.

    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Jun 13, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    4

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The refactoring of query syntax in artist_.$username.treatment.$productHandle.tsx changes -tag:"hide_from_combine" to tag_not:"hide-from-combine". Ensure that the new query correctly excludes items with the "hide-from-combine" tag as intended.

    Performance Concern:
    The new loader function in treatments.$productHandle._index.tsx introduces a significant amount of new code (over 600 lines). This could impact performance, especially on slower devices or networks. Consider optimizing or breaking down the function to improve maintainability and performance.

    Missing Tests:
    The PR lacks tests for the new functionality and refactored query syntax. Adding tests would help ensure that the changes do not break existing functionality and meet the new requirements.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the query syntax to avoid potential errors and ensure proper functionality

    Replace the deprecated syntax -tag:"hide_from_combine" with the correct syntax
    tag_not:"hide-from-combine" in the query string to ensure the query functions as intended
    without syntax errors.

    app/routes/artist_.$username.treatment.$productHandle.tsx [139]

    -query: `tag:"locationid-${locationId}" AND tag:"scheduleid-${product.scheduleId?.reference?.handle}" AND tag:"treatments" AND -tag:"hide_from_combine"`,
    +query: `tag:"locationid-${locationId}" AND tag:"scheduleid-${product.scheduleId?.reference?.handle}" AND tag:"treatments" AND tag_not:"hide-from-combine"`,
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies and fixes a deprecated syntax issue that could lead to query errors, ensuring the query functions as intended.

    10
    Add error handling for potentially invalid product IDs to prevent runtime errors

    Add error handling for the case where the parseGid function returns an invalid or
    undefined id, which could lead to runtime errors when constructing the filters array.

    app/routes/treatments.$productHandle._index.tsx [103]

    -`tag:"parentid-${parseGid(product.id).id}"`,
    +const parsedId = parseGid(product.id).id;
    +if (!parsedId) {
    +  throw new Error('Invalid product ID');
    +}
    +`tag:"parentid-${parsedId}"`,
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for potentially invalid product IDs is a good practice to prevent runtime errors, enhancing the robustness of the code.

    8
    Possible issue
    Add error handling to the loader function to manage potential runtime errors

    Consider adding error handling for the asynchronous loader function to manage cases where
    the storefront.query might fail or return unexpected results. This will improve the
    robustness of the function and provide a better user experience by handling potential
    runtime errors gracefully.

    app/routes/treatments.$productHandle.tsx [42-45]

     export async function loader({params, context}: LoaderFunctionArgs) {
    -  const {productHandle} = params;
    -  const {storefront} = context;
    -  ...
    +  try {
    +    const {productHandle} = params;
    +    const {storefront} = context;
    +    ...
    +  } catch (error) {
    +    console.error('Failed to load data:', error);
    +    throw new Response('Internal Server Error', { status: 500 });
    +  }
     }
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling to the loader function is crucial for robustness and user experience. It ensures that potential runtime errors are managed gracefully, which is important for production code.

    9
    Ensure that pagination and filtering logic are appropriately handled if still required

    The removal of pagination and filtering logic from the loader function simplifies the code
    but ensure that this functionality is either handled elsewhere or is no longer needed
    based on the new requirements. If filtering and pagination are still required, consider
    implementing them in a way that aligns with the new code structure.

    app/routes/treatments.$productHandle.tsx [72-74]

    -const {collection: collectionFiltersOnly} = await storefront.query(
    +const {collection: collectionFiltersOnly, paginationInfo} = await storefront.query(
       FILTER_COLLECTION,
       {
    -}
    +    // Include necessary pagination and filtering parameters
    +  }
    +)
     
    Suggestion importance[1-10]: 8

    Why: Ensuring that pagination and filtering logic are appropriately handled is important for functionality. The suggestion correctly identifies a potential issue with the removed logic.

    8
    Prevent runtime errors by ensuring optional fields have default values or are checked for null

    Add null checks or default values for optional fields in
    'TreatmentCollectionQueryVariables' to prevent runtime errors when these fields are not
    provided.

    storefrontapi.generated.d.ts [5801-5802]

    -country?: StorefrontAPI.InputMaybe<StorefrontAPI.CountryCode>;
    -language?: StorefrontAPI.InputMaybe<StorefrontAPI.LanguageCode>;
    +country: StorefrontAPI.InputMaybe<StorefrontAPI.CountryCode> = StorefrontAPI.DefaultCountryCode;
    +language: StorefrontAPI.InputMaybe<StorefrontAPI.LanguageCode> = StorefrontAPI.DefaultLanguageCode;
     
    Suggestion importance[1-10]: 7

    Why: Adding default values or null checks for optional fields can prevent potential runtime errors, enhancing the robustness of the code. However, the suggestion assumes the existence of default values which may not be defined.

    7
    Enhancement
    Improve input validation by using a more specific type for 'query'

    Replace the generic 'String' scalar with a more specific type or enum for 'query' to
    ensure the input is validated against expected formats or values.

    storefrontapi.generated.d.ts [5790]

    -query: StorefrontAPI.Scalars['String']['input'];
    +query: StorefrontAPI.Scalars['ProductQuery']['input'];
     
    Suggestion importance[1-10]: 8

    Why: Using a more specific type for 'query' can improve input validation and ensure that the input adheres to expected formats, which is crucial for maintaining data integrity and preventing errors.

    8
    Use a more specific type for the sortKey variable to enhance type safety

    Consider using a more specific type for the sortKey variable instead of the generic
    ProductCollectionSortKeys to ensure that only valid sort keys related to the treatments
    context are used, enhancing type safety and reducing potential bugs.

    app/routes/treatments.$productHandle._index.tsx [87]

    -let sortKey: ProductCollectionSortKeys = 'RELEVANCE';
    +let sortKey: TreatmentSortKeys = 'RELEVANCE';  // Assuming TreatmentSortKeys is a more specific subset relevant to treatments
     
    Suggestion importance[1-10]: 6

    Why: Using a more specific type for sortKey enhances type safety, but the improvement is minor and assumes the existence of a more specific type.

    6
    Maintainability
    Refactor sorting logic into a separate function to improve readability and maintainability

    Refactor the repeated logic for setting sortKey and reverse based on the sort parameter
    into a separate function to improve code readability and maintainability.

    app/routes/treatments.$productHandle._index.tsx [90-100]

    -const sort = searchParams.get('sort');
    -if (sort === 'newest') {
    -  sortKey = 'CREATED';
    -  reverse = true;
    -} else if (sort === 'cheapest') {
    -  sortKey = 'PRICE';
    -  reverse = false;
    -} else if (sort === 'expensive') {
    -  sortKey = 'PRICE';
    -  reverse = true;
    +function determineSortSettings(sortParam) {
    +  switch (sortParam) {
    +    case 'newest':
    +      return { sortKey: 'CREATED', reverse: true };
    +    case 'cheapest':
    +      return { sortKey: 'PRICE', reverse: false };
    +    case 'expensive':
    +      return { sortKey: 'PRICE', reverse: true };
    +    default:
    +      return { sortKey: 'RELEVANCE', reverse: false };
    +  }
     }
    +const { sortKey, reverse } = determineSortSettings(searchParams.get('sort'));
     
    Suggestion importance[1-10]: 7

    Why: Refactoring the sorting logic into a separate function improves code readability and maintainability, but the existing code is already clear and functional.

    7
    Replace the unstable analytics import with a stable version to avoid future compatibility issues

    Replace the deprecated UNSTABLE_Analytics import with the stable version if available, or
    check for an alternative analytics implementation. Using unstable or deprecated features
    can lead to future compatibility issues.

    app/routes/treatments.$productHandle.tsx [15]

    -import {UNSTABLE_Analytics as Analytics} from '@shopify/hydrogen';
    +import {Stable_Analytics as Analytics} from '@shopify/hydrogen';  # Replace 'Stable_Analytics' with the actual stable version name
     
    Suggestion importance[1-10]: 7

    Why: Replacing deprecated imports is important for maintainability and future compatibility. However, the suggestion assumes the existence of a stable version without verifying it.

    7
    Use a more descriptive variable name than collectionFiltersOnly

    Consider using a more descriptive variable name than collectionFiltersOnly to enhance code
    readability and maintainability. A name that reflects the content or purpose of the data
    can make the code easier to understand and maintain.

    app/routes/treatments.$productHandle.tsx [72-74]

    -const {collection: collectionFiltersOnly} = await storefront.query(
    +const {collection: productFilters} = await storefront.query(
       FILTER_COLLECTION,
       {
     }
     
    Suggestion importance[1-10]: 6

    Why: Using more descriptive variable names improves code readability and maintainability. However, this is a minor improvement compared to functional or security-related changes.

    6
    Improve code readability and maintainability by refactoring nested structures into separate types

    Refactor the nested structures in 'TreatmentCollectionQuery' to separate types or
    interfaces to improve code readability and maintainability.

    storefrontapi.generated.d.ts [5805-5806]

    -products: {
    -  nodes: Array<
    -    Pick<StorefrontAPI.Product, 'id' | 'title' | 'handle' | 'productType'> & {
    -      variants: {
    -        nodes: Array<
    -          Pick<StorefrontAPI.ProductVariant, 'id'> & {
    -            compareAtPrice?: StorefrontAPI.Maybe<
    -              Pick<StorefrontAPI.MoneyV2, 'amount' | 'currencyCode'>
    -            >;
    -            price: Pick<StorefrontAPI.MoneyV2, 'amount' | 'currencyCode'>;
    -          }
    -        >;
    -      };
    -    };
    -  >;
    -};
    +products: StorefrontAPI.ProductNodes;
     
    Suggestion importance[1-10]: 6

    Why: Refactoring nested structures into separate types can improve code readability and maintainability, but it is not a critical change and may require additional type definitions.

    6
    Best practice
    Enhance consistency and correctness by using a specific sort key type

    Consider using a more specific type for 'sortKey' to align with the expected product
    sorting keys, ensuring consistency with other API parts.

    storefrontapi.generated.d.ts [5800]

    -sortKey?: StorefrontAPI.InputMaybe<StorefrontAPI.ProductSortKeys>;
    +sortKey?: StorefrontAPI.InputMaybe<StorefrontAPI.TreatmentProductSortKeys>;
     
    Suggestion importance[1-10]: 7

    Why: Using a more specific type for 'sortKey' can enhance consistency and correctness across the API, although the improvement is relatively minor.

    7

    @jamalsoueidan jamalsoueidan merged commit e9b6ede into main Jun 13, 2024
    4 checks passed
    @jamalsoueidan jamalsoueidan deleted the change-filters-to-query branch June 13, 2024 22:15
    This pull request was closed.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant