Skip to content

@W-18820722 Refactor PDP for better readability and maintainability#2668

Merged
kevinxh merged 20 commits intofeature/chakra-ui-upgrade-v3from
@W-18820722-refactor-pdp
Jun 27, 2025
Merged

@W-18820722 Refactor PDP for better readability and maintainability#2668
kevinxh merged 20 commits intofeature/chakra-ui-upgrade-v3from
@W-18820722-refactor-pdp

Conversation

@kevinxh
Copy link
Contributor

@kevinxh kevinxh commented Jun 27, 2025

Description

This PR refactors the PDP implementation to split the giant index component into separate components and hooks for better maintainability.

Major changes

  • recommended product section moved to separate component
  • business logic moved to use-product-detail-data hook
  • product detail section is moved to
    • regular product -> /partials/product-details-simple.jsx
    • set/bundle product, /partials/product-details-composite.jsx
  • metadata -> page-metadata.jsx
  • cache -> page-cache.jsx
  • analytics -> page-analytics.jsx

Note: since this is the first page being refactored, I'd love to keep it simple to not overly optimize it. Once we do the similar work on other pages, we will be able to clearly see the patterns and optimize further.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

How to Test-Drive This PR

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@kevinxh kevinxh requested a review from a team as a code owner June 27, 2025 05:24
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Jun 27, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw a few broken things when testing the PDP as described in the PR description:

  1. the useDataCloud call seems to run more than once. I expected to see an error once in the console. The error itself I believe is already fixed in the develop branch. But in this case, seeing multiple errors perhaps indicate that the hook is called on every render?

Arc 2025-06-27 at 13 36 38@2x

  1. Both product bundles and sets are broken. Clicking on the main CTA button does not do anything. It returns an error in the console.

Arc 2025-06-27 at 13 38 14

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is I feel the useProductDetailData returns too much. But anyways, I leave it up to you whether that's worth updating or not. If yes, then perhaps we can also create a new ticket to address it later.

But other than that, the PR looks good to me 👍 Thanks for fixing a few things reported earlier.

Comment on lines +52 to +74
<ProductDetails
product={product}
primaryCategory={primaryCategory}
isProductASet={isProductASet}
isProductABundle={isProductABundle}
comboProduct={comboProduct}
childProductRefs={childProductRefs}
childProductSelection={childProductSelection}
setChildProductSelection={setChildProductSelection}
childProductOrderability={childProductOrderability}
setChildProductOrderability={setChildProductOrderability}
selectedBundleQuantity={selectedBundleQuantity}
setSelectedBundleQuantity={setSelectedBundleQuantity}
// Handlers
handleAddToCart={handleAddToCart}
handleAddToWishlist={handleAddToWishlist}
handleProductSetAddToCart={handleProductSetAddToCart}
handleProductBundleAddToCart={handleProductBundleAddToCart}
handleChildProductValidation={handleChildProductValidation}
// Loading states
isProductLoading={isProductLoading}
isBasketLoading={isBasketLoading}
isWishlistLoading={isWishlistLoading}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may complain about the prop drilling here. But I kinda like it.

It reminds me of the old days of getProps, which made it clear of where the data fetching was. And it wasn't spread out across many components.

Comment on lines +19 to +39
product,
isProductLoading,
primaryCategory,
isProductASet,
isProductABundle,
comboProduct,
childProductRefs,
childProductSelection,
setChildProductSelection,
childProductOrderability,
setChildProductOrderability,
selectedBundleQuantity,
setSelectedBundleQuantity,
handleAddToCart,
handleAddToWishlist,
handleProductSetAddToCart,
handleProductBundleAddToCart,
handleChildProductValidation,
isBasketLoading,
isWishlistLoading
} = useProductDetailData()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like the hook returns a bit too many stuffs. I think we can consider breaking it down into groups that make sense conceptually.

One way how that could work, according to AI:


🧩 Current Hook Analysis

The useProductDetailData hook currently handles 6 major responsibilities and returns 22 different values, making it a "God Hook" that violates the single responsibility principle.

📊 Proposed Conceptual Breakdown

1. Core Product Data Hook (useProductData)

// Handles basic product and category fetching
const useProductData = () => {
    // Product fetching (lines 45-68)
    // Category fetching (lines 74-80)
    // Error handling (lines 115-137)
    // Primary category state management (lines 139-148)
    // Variant URL management (lines 150-160)
    
    return {
        product,
        isProductLoading,
        primaryCategory,
        isProductASet: product?.type.set,
        isProductABundle: product?.type.bundle
    }
}

Used by: All components (Simple and Composite)

2. Bundle/Set State Hook (useBundleSetState)

// Handles complex bundle and set state management
const useBundleSetState = (product) => {
    // Child product selection state (lines 81-83)
    // Child product orderability state (line 82)
    // Bundle quantity state (line 84)
    // Bundle children data fetching (lines 89-111)
    // Combo product normalization (line 113)
    
    return {
        comboProduct,
        childProductRefs,
        childProductSelection,
        setChildProductSelection,
        childProductOrderability,
        setChildProductOrderability,
        selectedBundleQuantity,
        setSelectedBundleQuantity
    }
}

Used by: Only CompositeProductDetails

3. Cart Operations Hook (useProductCartActions)

// Handles all cart-related operations
const useProductCartActions = (product, bundleState) => {
    // Basic add to cart (lines 162-209)
    // Child product validation (lines 211-240)
    // Set add to cart (lines 242-247)
    // Bundle add to cart (lines 249-306)
    
    return {
        handleAddToCart,
        handleProductSetAddToCart,
        handleProductBundleAddToCart,
        handleChildProductValidation,
        isBasketLoading
    }
}

Used by: All components with different handlers

4. Wishlist Operations (useProductWishlist)

// Already extracted - handles wishlist operations
const useProductWishlist = () => {
    return {
        handleAddToWishlist,
        isWishlistLoading
    }
}

Used by: All components

🎯 Component Data Requirements Map

SimpleProductDetails needs:

  • useProductData() - Core product info
  • useProductCartActions() - Basic cart operations
  • useProductWishlist() - Wishlist operations

CompositeProductDetails needs:

  • useProductData() - Core product info
  • useBundleSetState() - Complex bundle/set state
  • useProductCartActions() - All cart operations
  • useProductWishlist() - Wishlist operations

📈 Benefits of This Breakdown

🟢 Reduced Complexity

  • Each hook has a single, clear responsibility
  • Easier to test individual concerns
  • Simpler mental model for developers

🟢 Better Reusability

  • useProductData could be reused on other product pages
  • useBundleSetState only loaded when needed
  • Cart operations could be shared across pages

🟢 Improved Performance

  • Bundle/set logic only executes for composite products
  • Smaller re-render scope when state changes
  • Better code splitting opportunities

🟢 Enhanced Maintainability

  • Bundle logic isolated from simple product logic
  • Each hook can evolve independently
  • Clearer debugging when issues arise

🔧 Implementation Strategy

// New main hook that composes smaller hooks
const useProductDetailData = () => {
    const productData = useProductData()
    const bundleState = useBundleSetState(productData.product)
    const cartActions = useProductCartActions(productData.product, bundleState)
    const wishlistActions = useProductWishlist()
    
    return {
        // Core data (5 values)
        ...productData,
        
        // Bundle/set data (8 values) - only when needed
        ...(productData.isProductASet || productData.isProductABundle ? bundleState : {}),
        
        // Actions (5 values)
        ...cartActions,
        ...wishlistActions
    }
}

🚨 Current Pain Points Addressed

  1. 22 return values → 4 focused hooks with clear boundaries
  2. 343 lines → ~80 lines per hook with single responsibilities
  3. Everything coupled → Independent concerns that can be tested/maintained separately
  4. Bundle logic always loaded → Conditional loading for better performance

This breakdown follows the principle of locality - related concerns stay together, unrelated concerns are separated, and each hook has a clear, focused purpose that matches how the UI components actually consume the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will follow up on this when i'm back next week, awesome feedback!

Comment on lines +12 to +20
const ProductDetails = (props) => {
const {isProductASet, isProductABundle} = props

if (isProductASet || isProductABundle) {
return <CompositeProductDetails {...props} />
}

return <SimpleProductDetails {...props} />
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: how about merging this back into the, um, parent product details component? This isn't much code, so I think it doesn't need to be in its own component.

@kevinxh kevinxh merged commit b894ac5 into feature/chakra-ui-upgrade-v3 Jun 27, 2025
33 checks passed
@kevinxh kevinxh deleted the @W-18820722-refactor-pdp branch June 27, 2025 22:45
alexvuong pushed a commit that referenced this pull request Jul 16, 2025
…actor-pdp

@W-18820722 Refactor PDP for better readability and maintainability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants