[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'

[HHHindawy]

Thanks for working on this, I really like how it looks!
Just left you a couple of comments 🙏

[HHHindawy]

Would the type ever be anything other than color or image?
We have it already typed as one of these values: type: 'color' | 'image';

[esezen]

Non TS users 😢

[HHHindawy]

Is it intentional that we disable the interactivity of the filter option if the checkbox is not visible?
Removing the <input> means that clicking on the filter option will do nothing and the onChange will not fire. Just asking to see if this is intentional.

[esezen]

Thanks for catching this! 27bc3af

[HHHindawy]

Minor thing: Should we create a renderProps variable that includes all of these props, similar to the rest of the components?

[esezen]

Let's discuss in Slack

[HHHindawy]

Same renderProps question

[HHHindawy]

How do you feel about adding an endContent as well? 🤔

[esezen]

I kinda like it but I would like to do it in a separate PR if that's okay

[HHHindawy]

I don't believe we have this prop defined anywhere

[esezen]

It's the legacy name I used. Then I checked it to something else. Thanks for the catch! 94dea79

[HHHindawy]

onChange is required, but it's missing from all stories.

Also can we add argTypes?

[esezen]

94dea79

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The composition pattern is well-executed — FilterOptionVisual cleanly wraps FilterOption via the startContent slot, avoiding duplication, and the test coverage is thorough across all three new components.

🚨 Critical Issues

• [File: src/components/chip.tsx Line: L52-L98] renderContent() can return undefined when type is neither 'color' nor 'image' (even though TypeScript narrows this away, the runtime path exists). The function has three if branches but no final return, so if somehow an unexpected type value is passed, undefined is returned and passed as children to RenderPropsWrapper. The fallback condition on line 54 already checks !['color', 'image'].includes(type), but TypeScript may not always enforce type at the call site (e.g., data from an API). Add an explicit return null at the end of renderContent:

  // after the last if (type === 'image') block
  return null;

• [File: src/components/chip.tsx Line: L54] The check !['color', 'image'].includes(type) is redundant given that type is typed as 'color' | 'image'. The only meaningful condition for the fallback is !value || value.trim() === ''. Remove the dead branch to prevent confusing future readers and to ensure the TypeScript narrowing on lines 66 and 79 (if (type === 'color') / if (type === 'image')) remains exhaustive:

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

⚠️ Important Issues

• [File: src/components/filter-option.tsx Line: L4] Missing blank line between import statements and the const baseClasses declaration. All other files in this codebase include a blank line between the last import and the first declaration. Minor, but inconsistent with project style.

• [File: src/components/chip.tsx Line: L91-L94] The onError handler on the <img> directly mutates the DOM (e.currentTarget.style.display, e.currentTarget.parentElement?.classList.add(...)) rather than using React state. This bypasses React's rendering cycle, can cause issues with server-side rendering, and may be overwritten on subsequent renders. Use a React state variable to handle the error:

  const [imgError, setImgError] = React.useState(false);
  // ...
  {!imgError ? (
    <img src={value} alt={name} className='w-full h-full object-cover' onError={() => setImgError(true)} />
  ) : null}
  // Apply 'bg-gray-200' conditionally via cn() on the wrapper div

• [File: src/components/filter-option--visual.tsx Line: L27] data-slot='visual-filter-option' is hardcoded as a JSX prop on the <FilterOption> call, but FilterOption spreads ...props onto its <li> element. This means the data-slot from FilterOptionVisual will overwrite the data-slot='filter-option' set on line 90 of filter-option.tsx. This is likely intentional, but it means any consumer reading data-slot on the list item will see 'visual-filter-option' — which is correct — but this override behaviour should be documented, since it's non-obvious.

• [File: src/components/filter-option.tsx Line: L93] The <label> has a hardcoded text-sm class while the parent <li> has text-base from baseClasses. This creates an inconsistency: the li declares text-base but the label inside immediately overrides it to text-sm. Either remove text-base from baseClasses and keep text-sm on the label, or remove text-sm from the label and let it inherit text-base.

• [File: src/components/filter-option.tsx Line: L46-L71] startContent and onChange are included in the renderProps useMemo dependency array, but both are reference types (a ReactNode and a function) that will cause the memo to recompute on every render unless the consumer memoizes them. This is not a bug, but consumers passing inline JSX as startContent or an inline arrow as onChange will get no memoization benefit. Consider documenting this expectation, or restructuring.

• [File: src/index.ts Line: L22] FilterOptionVisualProps is exported but FilterOptionVisualProps does not re-export ChipVariants or ChipProps, even though Chip is a public component used as startContent within FilterOptionVisual. Consumers who want to customize the swatch size or type have no way to reference ChipVariants directly — however ChipVariants and ChipProps are already exported on line 21, so this is fine. No action needed here.

💡 Suggestions

• [File: src/components/chip.tsx Line: L54] The fallback renders a white bg-white div, but a white color swatch is visually indistinguishable from the fallback state. Consider a dashed border or a diagonal strikethrough pattern (via CSS) to distinguish "no color provided" from an intentional white swatch.

• [File: src/components/filter-option.tsx Line: L75] The SVG checkmark path is hardcoded inline. As the codebase grows, consider extracting it to a shared icon component or a utility to enable reuse and consistent updates.

• [File: src/stories/components/Chip/Chip.stories.tsx Line: L110] The @ts-expect-error: Composed types comment suppresses a legitimate type error caused by the stories object shape not matching componentOverrides. This pattern also appears in FilterOption.stories.tsx L164. Consider structuring the story args so the override is passed directly without needing @ts-expect-error, which would make the intended API clearer in the stories.

• [File: src/components/filter-option--visual.tsx] FilterOptionVisual does not expose its own componentOverrides for the inner Chip swatch. A consumer who wants to customize only the swatch must use FilterOption directly with a custom startContent. This is acknowledged in the PR description (asking whether sub-component overrides are needed), but documenting the decision either way in a comment would help future contributors.

Overall Assessment: ⚠️ Needs Work

[HHHindawy]

LGTM!