Implemented centralized dynamic DxpProductFeatures component(471-bopis) by ashutosh7i · Pull Request #367 · hotwax/dxp-components

ashutosh7i · 2025-02-26T13:23:08Z

Related Issues

Short Description and Why It's Useful

This DxpProductFeatures component can be used to render the ProductFeatureType and FeatureOptions for a given product or groupid.
Previously in (hotwax/bopis#471) and other places it was hardcoded to "color" and "size" FeatureTypes, which was not handling other possible FeatureTypes that might be in a product.

To use this-

import the DxpProductFeatures component from "@hotwax/dxp-components"; as we import other components.
place it in the markup where the component is to be rendered. <DxpProductFeatures :productGroupId="currentProductId" @selected_variant="receiveEmit"/>
pass the productId or groupId and handle the selected variant by user emitted by @selected_variant

Working-

It uses the passed prop to fetch the variant products and stores them in state for caching.
reads the productFeatures key and its values from the first product (virtual product), calculates all possible valid variants.
renders the ProductFeatureType and FeatureOptions using , and
when user selects or alters a variant and or option, it emits the varientId of that respective product.

The creation of this component was requested in this PR- hotwax/bopis#471.

Screenshots of Visual Changes before/after (If There Are Any)

Is the changes contains any breaking change?

If there are any breaking change include those in the release notes file

Yes
No

Contribution and Currently Important Rules Acceptance

I read and followed contribution rules

…on selection emits the product id of selected variant.

…cy. (#471_productFeatures)

ymaheshwari1 · 2025-05-02T06:30:31Z

@@ -0,0 +1,119 @@
+<template>
+  <div>
+    <ion-spinner v-if="isLoading" name="crescent" />


We won't add a spinner, instead we can think on adding skeleton-text.

ymaheshwari1 · 2025-05-02T06:37:50Z

+const selectedProductId = ref('');
+
+onMounted(async () => {
+  const products = await productFeatureStore.fetchProductsByGroupId(props.productGroupId);


I think we should get the product as a prop instead of making an api call here, as the app will maintain the state and also cache the same.

ymaheshwari1 · 2025-05-02T06:38:28Z

+const emit = defineEmits(['selected_variant']);
+
+const productFeatureStore = useProductFeatureStore();
+const isLoading = computed(() => productFeatureStore.isLoading);


Maintain isLoading variable on component level.

ymaheshwari1 · 2025-05-02T06:46:47Z

+  if (selectedVariant) {
+    selectedProductId.value = selectedVariant.productId;
+  }
+  emit('selected_variant', selectedVariant.productId);


This will lead to undefined error, if in any case the variant is not found.

dt2patel · 2025-10-15T07:19:33Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new, dynamic DxpProductFeatures component, which is a great improvement over hardcoded feature handling. The implementation is solid, with a new Pinia store for caching and a utility for sorting apparel sizes. My review focuses on improving type safety by replacing any types with specific interfaces, fixing a critical bug related to handling undefined variants, and addressing some minor code style and dead code issues. Overall, these changes make the codebase more robust and maintainable.

gemini-code-assist · 2025-10-15T07:21:11Z

+  if (selectedVariant) {
+    selectedProductId.value = selectedVariant.productId;
+  }
+  emit('selected_variant', selectedVariant.productId);


If findMatchingVariant returns undefined, accessing selectedVariant.productId will cause a runtime TypeError. This is a critical issue that will crash the component's logic. You should use optional chaining (?.) to safely access the productId property.

emit('selected_variant', selectedVariant?.productId);

gemini-code-assist · 2025-10-15T07:21:11Z

+  extractProductFeatures(products);
+});
+
+function extractProductFeatures(products: any[]) {


The products parameter is typed as any[], which bypasses TypeScript's type checking. To improve type safety and code maintainability, you should define a Product interface and use it here. This would also benefit other functions like findMatchingVariant.

For example:

interface Product { productId: string; productFeatures: string[]; isVariant: string; // ... other properties from the API response } function extractProductFeatures(products: Product[]) { // ... }

gemini-code-assist · 2025-10-15T07:21:11Z

+
+export const useProductFeatureStore = defineStore('productFeature', {
+  state: () => ({
+    products: {} as Record<string, any[]>,


The products state is typed as Record<string, any[]>. Using any reduces type safety. It's best to define a shared Product interface (e.g., in src/types.ts) and use it consistently in both the store and components.

For example:

import type { Product } from '@/types'; // ... state: () => ({ products: {} as Record<string, Product[]>, // ... }),

gemini-code-assist · 2025-10-15T07:21:11Z

+
+//////////////////////////////////////////////////////////////////
+
+export function sortSizes(sizes: any) {


This function, and others in this file, use any as a type, which is not ideal in a TypeScript project. To leverage TypeScript's static analysis and improve code quality, you should provide explicit types for parameters and return values.

Suggested change

export function sortSizes(sizes: any) {

export function sortSizes(sizes: string[]): string[] {

gemini-code-assist · 2025-10-15T07:21:11Z

+const props = defineProps(['productGroupId']);
+const emit = defineEmits(['selected_variant']);


For better type safety and clarity in a TypeScript setup, it's recommended to use the generic-based syntax for defineProps and defineEmits. This allows for better type inference and compile-time checks.

const props = defineProps<{ productGroupId: string; }>(); const emit = defineEmits<{ (e: 'selected_variant', productId: string | undefined): void; }>();

gemini-code-assist · 2025-10-15T07:21:11Z

+const isLoading = computed(() => productFeatureStore.isLoading);
+const productFeatures = ref({} as Record<string, string[]>);
+const selectedFeatures = ref({} as Record<string, string>);
+const selectedProductId = ref('');


The reactive variable selectedProductId is assigned a value but is never read. This is dead code and should be removed to improve clarity. The assignments on lines 98 and 106 should also be removed.

gemini-code-assist · 2025-10-15T07:21:11Z

+      } catch (error) {
+        return [];
+      } finally {


The catch block returns an empty array but swallows the error. This prevents the consumer of this action from knowing that an error occurred. It's good practice to log the error for debugging purposes.

} catch (error) { console.error("Failed to fetch products by group ID:", error); return []; }

gemini-code-assist · 2025-10-15T07:21:11Z

+    if (size.search(patterns[iteration].regex) >= 0) {
+      return { regex: patterns[iteration].regex, index: patterns[iteration].index, size: size, sizeVal: parseInt(size) || 0 };
+    }
+    return findRegexMatch(patterns, iteration = iteration + 1, size);


The recursive call here includes a side effect in its argument list (iteration = iteration + 1). This is unconventional and can be confusing. It's better to pass the new value directly. Additionally, consider converting this recursion to a simple loop for better readability.

Suggested change

return findRegexMatch(patterns, iteration = iteration + 1, size);

return findRegexMatch(patterns, iteration + 1, size);

gemini-code-assist

Code Review

This pull request introduces a new dynamic DxpProductFeatures component, which is a great improvement over hardcoded feature handling. The implementation is solid, but I've found a few areas for improvement regarding type safety, error handling, and correctness. My main concerns are a potential critical runtime error if a product variant isn't found, and some incorrect regular expressions in the new apparel sorting utility. I've also included several medium-severity suggestions to improve type safety by replacing any types with specific interfaces/types, which will make the code more robust and maintainable.

gemini-code-assist · 2025-10-15T07:24:02Z

+  if (selectedVariant) {
+    selectedProductId.value = selectedVariant.productId;
+  }
+  emit('selected_variant', selectedVariant.productId);


This line will cause a critical runtime error (TypeError: Cannot read properties of undefined (reading 'productId')) if findMatchingVariant returns undefined. This can happen if a valid variant for the selected features doesn't exist. You should use optional chaining to safely access productId and emit undefined in that case.

emit('selected_variant', selectedVariant?.productId);

gemini-code-assist · 2025-10-15T07:24:02Z

+  Object.keys(sortedFeatures).forEach(featureType => {
+    selectedFeatures.value[featureType] = sortedFeatures[featureType][0];
+  });


The code sortedFeatures[featureType][0] will throw a runtime error if sortedFeatures[featureType] is an empty array. You should add a check to ensure the array has elements before accessing the first one.

Object.keys(sortedFeatures).forEach(featureType => { if (sortedFeatures[featureType]?.length) { selectedFeatures.value[featureType] = sortedFeatures[featureType][0]; } });

gemini-code-assist · 2025-10-15T07:24:02Z

+  /^7x.*$/i,
+  /^XXXXXXXL.*$/i,
+  /^8x.*$/i,
+  /^XXXXXXXL.*$/i,


The regular expression for size 8x is the same as for 7x (^XXXXXXXL.*$/i), which is incorrect. The regex for 8x should contain eight 'X's. This off-by-one error seems to continue for all subsequent larger sizes (9x, 10x, etc.). Please correct these regular expressions to ensure proper size sorting.

Suggested change

/^XXXXXXXL.*$/i,

/^XXXXXXXXL.*$/i,

gemini-code-assist · 2025-10-15T07:24:02Z

+import { ref, defineProps, defineEmits, onMounted, computed } from 'vue';
+import { useProductFeatureStore } from '../store/productFeature';
+
+const props = defineProps(['productGroupId']);


For better type safety and to leverage TypeScript's features, it's recommended to use the object syntax with types for defineProps.

const props = defineProps<{ productGroupId: string }>();

gemini-code-assist · 2025-10-15T07:24:02Z

+import { useProductFeatureStore } from '../store/productFeature';
+
+const props = defineProps(['productGroupId']);
+const emit = defineEmits(['selected_variant']);


To ensure type safety for emitted events, it's best practice to provide a type literal to defineEmits. This also improves autocompletion and type inference in parent components. I've updated the signature to allow undefined for the product ID, which aligns with another suggestion to prevent runtime errors.

const emit = defineEmits<{ (e: 'selected_variant', id: string | undefined): void }>();

gemini-code-assist · 2025-10-15T07:24:02Z

+  extractProductFeatures(products);
+});
+
+function extractProductFeatures(products: any[]) {


The products parameter is typed as any[] here and in findMatchingVariant (line 111). Using any bypasses TypeScript's type checking. To improve type safety and code clarity, please define an interface for the product object and use it in both function signatures. Based on its usage, it could look something like this:

interface Product { productId: string; productFeatures: string[]; isVariant: string; }

The function signature would then become function extractProductFeatures(products: Product[]) {.

gemini-code-assist · 2025-10-15T07:24:03Z

+
+export const useProductFeatureStore = defineStore('productFeature', {
+  state: () => ({
+    products: {} as Record<string, any[]>,


The products state property is typed as Record<string, any[]>. Using any reduces type safety and makes the code harder to maintain. It would be beneficial to define a Product interface and use it here, for example: Record<string, Product[]>. This would align with the component that consumes this store.

gemini-code-assist · 2025-10-15T07:24:03Z

+      } catch (error) {
+        return [];
+      } finally {


The catch block currently returns an empty array, which silences the error. While this prevents a crash, it hides potential problems from developers during debugging and from the user. It's better to at least log the error to the console.

} catch (error) { console.error('Failed to fetch products by group ID:', error); return []; }

gemini-code-assist · 2025-10-15T07:24:03Z

+    if (size.search(patterns[iteration].regex) >= 0) {
+      return { regex: patterns[iteration].regex, index: patterns[iteration].index, size: size, sizeVal: parseInt(size) || 0 };
+    }
+    return findRegexMatch(patterns, iteration = iteration + 1, size);


The recursive call findRegexMatch(patterns, iteration = iteration + 1, size) uses an assignment as a side effect to increment iteration. It's a better practice in recursive functions to pass the new value directly without modifying the current scope's variables.

Suggested change

return findRegexMatch(patterns, iteration = iteration + 1, size);

return findRegexMatch(patterns, iteration + 1, size);

gemini-code-assist · 2025-10-15T07:24:03Z

+
+//////////////////////////////////////////////////////////////////
+
+export function sortSizes(sizes: any) {


This new utility file uses any in many function signatures, such as here. To leverage TypeScript's benefits, please provide specific types. For this function, the sizes parameter should be string[] and the return type should also be string[].

Suggested change

export function sortSizes(sizes: any) {

export function sortSizes(sizes: string[]): string[] {

ashutosh7i and others added 5 commits February 26, 2025 11:49

Implemented: DxpProductFeatures

8ffbfa2

Fixed: component takes product id prop in, fetched, displays, then up…

875b97f

…on selection emits the product id of selected variant.

Fixed: separated api logic into a store.

60e14d2

Fixed: removed console logs and indentation fixes.

9fe26e9

Merge branch 'hotwax:main' into 471_productFeatures

f517362

ashutosh7i requested a review from ymaheshwari1 February 26, 2025 13:23

ymaheshwari1 requested changes Mar 13, 2025

View reviewed changes

Comment thread src/store/productFeature.ts Outdated

Comment thread src/store/productFeature.ts Outdated

Comment thread src/store/productFeature.ts Outdated

Comment thread src/store/productFeature.ts Outdated

Comment thread tsconfig.json

ashutosh7i and others added 3 commits March 17, 2025 14:15

Merge branch 'hotwax:main' into 471_productFeatures

f2779d3

Fixed: update product feature store getters for clarity and consisten…

f809e57

…cy. (#471_productFeatures)

Merge branch 'hotwax:main' into 471_productFeatures

395bbba

ymaheshwari1 requested changes May 2, 2025

View reviewed changes

gemini-code-assist Bot reviewed Oct 15, 2025

View reviewed changes


		//////////////////////////////////////////////////////////////////

		export function sortSizes(sizes: any) {

	export function sortSizes(sizes: any) {
	export function sortSizes(sizes: string[]): string[] {

		const props = defineProps(['productGroupId']);
		const emit = defineEmits(['selected_variant']);

	return findRegexMatch(patterns, iteration = iteration + 1, size);
	return findRegexMatch(patterns, iteration + 1, size);

Conversation

ashutosh7i commented Feb 26, 2025

Related Issues

Short Description and Why It's Useful

Screenshots of Visual Changes before/after (If There Are Any)

Is the changes contains any breaking change?

Contribution and Currently Important Rules Acceptance

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ymaheshwari1 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

ymaheshwari1 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

ymaheshwari1 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

ymaheshwari1 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

dt2patel commented Oct 15, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!