[CDX-335] Feature: Create Visual Filter component (color swatch & image thumbnail)

[esezen]

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:

@HHHindawy @TarekAlQaddy @Mudaafi componentOverrides are not implemented at the moment for the row components. Do you think we need them in this PR?

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The data-slot value 'visual-filter-option-list-row' is inconsistent with the test expectation and comment on line 135 which expects 'visual-filter' based on the test description. Review for consistency.

Suggested change

	data-slot='visual-filter-option-list-row'
	data-slot='visual-filter'

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured component architecture with comprehensive test coverage, good accessibility implementation, and proper TypeScript type definitions.

🚨 Critical Issues

[File: src/components/chip.tsx Line: L91-L94]
The onError handler directly manipulates the DOM using style.display and classList.add(), which bypasses React's reconciliation and can cause inconsistent state. This approach is error-prone and doesn't follow React best practices.

onError={(e) => {
  e.currentTarget.style.display = 'none';
  e.currentTarget.parentElement?.classList.add('bg-gray-200');
}}

Recommendation: Use React state to manage the error condition:

const [imageError, setImageError] = useState(false);

// In render:
{type === 'image' && !imageError ? (
  <img src={value} alt={name} onError={() => setImageError(true)} />
) : (
  <div className="bg-gray-200" />
)}

---

[File: src/components/chip.tsx Line: L54]
The type validation !['color', 'image'].includes(type) is redundant since TypeScript already enforces type: 'color' | 'image' at compile time. This runtime check can never be true with valid TypeScript usage and only adds unnecessary code.

Recommendation: Remove the type check:

if (!value || value.trim() === '') {
  // fallback...
}

⚠️ Important Issues

[File: src/components/chip.tsx Line: L52-L99]
The renderContent function is recreated on every render but isn't memoized, despite the component already using useMemo for renderProps. While this isn't critical for a small component, it's inconsistent with the pattern established by the existing useMemo usage.

Recommendation: Either memoize renderContent with useCallback or inline it directly in the return statement. For consistency with other components in the codebase (like Badge and Button), consider inlining.

---

[File: src/components/filter-option-list-row.tsx Line: L48]
The duration value duration-250 in the Tailwind classes is non-standard. Tailwind CSS's default duration scale uses duration-75, duration-100, duration-150, duration-200, duration-300, etc. The value 250 doesn't exist in the default configuration.

Recommendation: Use standard Tailwind duration values:

className='... transition-all duration-200 ...'
// or
className='... transition-all duration-300 ...'

If duration-250 is intentionally configured in your Tailwind config, this can be ignored.

---

[File: src/components/filter-option-list-row.tsx Line: L47-L59]
The checkbox element is conditionally created using && which produces false when checkboxVisible is false. While React handles this correctly, it's cleaner to use null or extract the JSX into a conditional.

Recommendation:

const checkboxEl = checkboxVisible ? (
  <div className='cio-checkbox...'>
    {/* svg content */}
  </div>
) : null;

---

[File: src/components/filter-option-list-row.tsx Line: L48]
The CSS selector group-has-[input:checked]:shadow-[inset_0_0_0_32px_#000] uses a hardcoded color value #000 instead of a Tailwind color token. This breaks consistency with the theme system.

Recommendation: Use Tailwind color tokens:

className='... group-has-[input:checked]:shadow-[inset_0_0_0_32px_theme(colors.black)]'

Or define a custom utility in your Tailwind config for better reusability.

---

[File: src/components/visual-filter-option-list-row.tsx Line: L43]
The data-slot attribute overrides the parent component's data-slot value. Since this component wraps FilterOptionListRow, there will be two elements with data-slot attributes in the DOM hierarchy, which could cause confusion when debugging or styling.

Current behavior:

<li data-slot="visual-filter-option-list-row" class="cio-visual-filter-option-list-row">
  <!-- FilterOptionListRow also sets data-slot="filter-option-list-row" -->
</li>

Recommendation: Either remove the data-slot prop from the FilterOptionListRow call or use a different attribute name to maintain both slot identifiers without conflict.

💡 Suggestions

[File: src/components/chip.tsx Line: L52-L99]
The component has three separate return paths (fallback, color, image), causing significant code duplication with the same data-slot, className, aria-label, and role props repeated three times. Consider extracting common props:

const commonProps = {
  'data-slot': 'chip',
  className: cn(chipVariants({ size, className })),
  'aria-label': name,
  role: 'img' as const,
  ...props,
};

if (type === 'color') {
  return <div {...commonProps} style={{ backgroundColor: value }} />;
}
// etc.

---

[File: spec/components/Chip/Chip.test.tsx Line: L15]
The test checks for backgroundColor conversion to RGB format (rgb(255, 0, 0)), which tests browser behavior rather than component logic. While not incorrect, it creates a tight coupling to browser CSS parsing.

Suggestion: Consider testing the style attribute directly:

expect(element).toHaveStyle({ backgroundColor: '#FF0000' });

This is more resilient to different testing environments.

---

[File: spec/components/FilterOptionListRow/FilterOptionListRow.test.tsx Line: L146]
Using document.querySelector('.cio-checkbox') is less robust than using testing-library queries. While this works, it bypasses the accessibility-focused querying that testing-library encourages.

Suggestion: If the checkbox needs to be verified, consider adding a test id or using role-based queries. However, since this is testing implementation details (CSS classes), this test might be better suited as a visual regression test in Storybook.

---

[File: All test files]
Test descriptions use test instead of it, which is fine but inconsistent with common conventions. The vitest docs and most React testing examples use it for better readability ("it renders...").

Suggestion: Consider using it for consistency with the wider ecosystem:

it('renders with display value', () => { ... })

---

[File: src/components/chip.tsx Line: L87-L95]
The image has an onError handler that modifies parent DOM elements. Even with the state-based solution recommended above, consider adding a visual error state (icon, text) to better communicate to users that the image failed to load, rather than just showing gray.

---

[File: src/components/filter-option-list-row.tsx Line: L48-L55]
The SVG checkmark is inlined directly in the component. For better maintainability and reusability, consider extracting it to a shared icon component or constant.

Overall Assessment: ⚠️ Needs Work

The PR introduces well-tested, accessible components with good TypeScript definitions. However, there is one critical issue with direct DOM manipulation in the Chip component that should be addressed before merge. The other issues are important for code quality, consistency, and maintainability but are not blockers.