Add Support For Single Type Facets

[VincentGalloro]

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

• I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
• I have added JSDoc comments to my TypeScript definitions for improved documentation.
• I have added tests that prove my fix is effective or that my feature works.
• I have added any necessary documentation (if appropriate).
• I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no API changes)
• Documentation content changes
• TypeScript type definitions update
• Other... Please describe:

[VincentGalloro]

Code Review Results

✅ Strengths

Well-structured implementation with comprehensive test coverage for single-type facet functionality.

🚨 Critical Issues

None identified.

⚠️ Important Issues

[File: src/components/Filters/UseFilterOptionsList.tsx Line: 28-29] The logic for single facet selection has a subtle bug. When clicking an already selected option in a single facet, the code creates an empty object {} but then sets newMap[optionValue] = !selectedOptionMap[optionValue], which evaluates to true (since the option was selected). This means clicking a selected option will deselect it briefly but then immediately re-select it instead of truly deselecting it.

Issue: The line reads the old state (selectedOptionMap[optionValue]) after clearing the map for single facets.

Suggested fix:

const onOptionSelect = (optionValue: string) => {
  const isCurrentlySelected = selectedOptionMap[optionValue];
  const newMap = facet.type === 'multiple' ? { ...selectedOptionMap } : {};
  newMap[optionValue] = !isCurrentlySelected;

  const selectedOptions = Object.keys(newMap).filter((key) => newMap[key]);
  setSelectedOptionMap(newMap);
  modifyRequestMultipleFilter(selectedOptions.length ? selectedOptions : null);
};

This captures the current state before clearing the map, ensuring correct toggle behavior.

[File: src/components/Filters/FilterGroup.tsx Line: 35] The prop name multipleFacet is misleading since it now accepts both multiple and single facets. Consider renaming to facet or optionsFacet for clarity. This also applies to the hook interface at src/components/Filters/UseFilterOptionsList.tsx:6 and throughout the component.

💡 Suggestions

[File: src/components/Filters/UseFilterOptionsList.tsx Line: 28] Consider extracting the facet type check into a helper function for better readability:

const shouldPreserveExistingSelections = (facetType: string) => facetType === 'multiple';

const onOptionSelect = (optionValue: string) => {
  const isCurrentlySelected = selectedOptionMap[optionValue];
  const newMap = shouldPreserveExistingSelections(facet.type) ? { ...selectedOptionMap } : {};
  newMap[optionValue] = !isCurrentlySelected;
  // ...
};

[File: spec/components/Filters/Filters.test.jsx Line: 805-833] The test "Single Facet: Selecting an option should de-select other options" could be enhanced by verifying the URL state or filter state is correctly updated, not just the checkbox UI state. This would provide more comprehensive coverage of the feature.

Overall Assessment: ⚠️ Needs Work

The implementation is well-tested and adds valuable functionality, but the toggle logic bug in UseFilterOptionsList.tsx:28-29 needs to be fixed before merging to ensure single facets behave correctly when deselecting options.

From Claude:

The reviewer is mistaken. Here's the explanation:

---

This is not a bug. The original code works correctly because selectedOptionMap is read before any mutation
occurs.

Let's trace through the execution when clicking an already-selected option (e.g., "small" is selected):

const onOptionSelect = (optionValue: string) => {
const newMap = facet.type === 'multiple' ? { ...selectedOptionMap } : {};
newMap[optionValue] = !selectedOptionMap[optionValue];
// ...
};

State before click: selectedOptionMap = { small: true }

Step-by-step execution:

1. newMap = {} (for single facet, creates a new empty object)
2. newMap["small"] = !selectedOptionMap["small"] → newMap["small"] = !true → newMap["small"] = false
3. newMap = { small: false }
4. selectedOptions = Object.keys({ small: false }).filter(key => newMap[key]) → [] (empty array, since false is
   filtered out)

Result: The option is correctly deselected.

Why the reviewer's concern is incorrect:

The reviewer suggested that reading selectedOptionMap[optionValue] after clearing the map would cause issues.
However, line 28 creates a new object ({}) and assigns it to newMap - it does not mutate selectedOptionMap. The
original selectedOptionMap state object remains unchanged and accessible until setSelectedOptionMap(newMap) is
called, which happens after we've already read from it.

In JavaScript, const newMap = {} creates a completely separate object. Reading selectedOptionMap[optionValue] on
line 29 still references the original state object, which correctly contains { small: true }.

[esezen]

This is looking good overall. I requested one change. Thank you!

[esezen]

This is a bit confusing. Can we please rename the prop from multipleFacet to something else? We would have to keep backwards compatibility but we can at least use the new name in our implementation to make it easier to maintain

[VincentGalloro]

@esezen I've renamed the prop to "facet" and left in backwards compatibility incase "multipleFacet" is still passed

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured implementation with comprehensive test coverage for single facet selection behavior and backward compatibility for the deprecated prop.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/components/Filters/UseFilterOptionsList.tsx Line: L35-L36]
Logic error in single facet selection: When a user clicks an already-selected option in a single facet, the code creates an empty map but still uses the OLD selectedOptionMap[optionValue] value to toggle. This causes the newly selected option to remain checked instead of being deselected.

const newMap = facet.type === 'multiple' ? { ...selectedOptionMap } : {};
newMap[optionValue] = !selectedOptionMap[optionValue]; // Bug: uses OLD map

For single facets, when selectedOptionMap[optionValue] is true, this creates an empty newMap, then sets newMap[optionValue] = !true = false, resulting in an empty map (all unchecked). This is correct.

However, when clicking a different option where selectedOptionMap[optionValue] is false or undefined, it creates an empty newMap and sets newMap[optionValue] = !false = true, which is correct.

But when clicking the same selected option again to deselect it, it creates an empty newMap, and then checks selectedOptionMap[optionValue] which is true, so it sets newMap[optionValue] = !true = false. The result is an empty map, which works correctly.

Actually, upon further analysis, the logic appears to work correctly based on your test at spec/components/Filters/Filters.test.jsx:L834-L850. However, the code is confusing because it reads from the old map after clearing it. Consider this refactor for clarity:

const onOptionSelect = (optionValue: string) => {
  const isCurrentlySelected = selectedOptionMap[optionValue];
  const newMap = facet.type === 'multiple' ? { ...selectedOptionMap } : {};
  newMap[optionValue] = !isCurrentlySelected;

  const selectedOptions = Object.keys(newMap).filter((key) => newMap[key]);
  setSelectedOptionMap(newMap);
  modifyRequestMultipleFilter(selectedOptions.length ? selectedOptions : null);
};

This makes it clear that we're checking the selection state BEFORE clearing the map.

[File: spec/components/Filters/UseFilterOptionsList.test.jsx Line: L1]
Incorrect filename: The test file is named UseFilterOptionsList.test.jsx but it's testing the FilterOptionsList component, not the useFilterOptionsList hook. The filename should be FilterOptionsList.test.jsx to match the component being tested. Alternatively, if testing the hook, use renderHook from @testing-library/react.

[File: spec/components/Filters/UseFilterOptionsList.test.jsx Line: L147]
Trailing comma inconsistency: There's an extra blank line before the closing brace with a trailing comma on L145. This is inconsistent with the codebase style.

💡 Suggestions

[File: src/components/Filters/FilterOptionsList.tsx Line: L8]
Missing prop access: The component destructures facet from the hook return value but doesn't receive it directly from props. While this works, it would be clearer to destructure facet from props at the function signature level for better readability:

export default function FilterOptionsList(props: UseFilterOptionsListProps) {
  const facet = 'facet' in props ? props.facet : props.multipleFacet;
  const {
    initialNumOptions,
    // ... rest
  } = useFilterOptionsList(props);

[File: src/types.ts Line: L357-L360]
Add JSDoc for PlpSingleFacet: Consider adding JSDoc comments to explain the difference between PlpSingleFacet and PlpMultipleFacet for future developers:

/**
 * Single facet type allows only one option to be selected at a time.
 * Selecting a new option will automatically deselect the previously selected option.
 */
export interface PlpSingleFacet extends PlpFacet {
  type: 'single';
  options: Array<PlpFacetOption>;
}

Overall Assessment: ⚠️ Needs Work