Skip to content

Enhance Select component value type #135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Enhance Select component value type #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@lambdacurry/forms",
"version": "0.19.5",
"version": "0.20.0",
"type": "module",
"main": "./dist/index.js",
"types": "./dist/index.d.ts",
Expand All @@ -26,9 +26,7 @@
"import": "./dist/ui/*.js"
}
},
"files": [
"dist"
],
"files": ["dist"],
"scripts": {
"prepublishOnly": "yarn run build",
"build": "vite build",
Expand Down
19 changes: 10 additions & 9 deletions packages/components/src/ui/select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import { useOverlayTriggerState } from 'react-stately';
import { PopoverContent, PopoverTrigger } from './popover';
import { cn } from './utils';

export interface SelectOption {
export interface SelectOption<T = string> {
label: string;
value: string;
value: T;
}
Comment on lines +8 to 11
Copy link

@coderabbitai coderabbitai bot Aug 25, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Constrain T to React.Key to avoid key collisions and invalid data- attribute values*

Allowing any T (including objects) introduces concrete problems:

  • React keys: String(object) collapses to "[object Object]" causing duplicate keys and unstable reconciliation.
  • Equality: o.value === value is unreliable for objects across renders.
  • DOM data-* attributes: React types expect string | number; arbitrary T may not be assignable.

Constrain T to React.Key (string | number) to keep the API flexible while safe.

Apply this diff:

-export interface SelectOption<T = string> {
+export interface SelectOption<T extends React.Key = string> {
   label: string;
-  value: T;
+  value: T;
 }

If you truly need object values, consider adding a getKey: (value: T) => React.Key and equals?: (a: T, b: T) => boolean; but constraining to React.Key is the simplest, safest path for now.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export interface SelectOption<T = string> {
label: string;
value: string;
value: T;
}
export interface SelectOption<T extends React.Key = string> {
label: string;
value: T;
}
🤖 Prompt for AI Agents 
In packages/components/src/ui/select.tsx around lines 8 to 11, the SelectOption
generic should be constrained to React.Key to prevent objects from being used as
values (which breaks React keys, equality checks, and data-* typing); change the
interface generic to extend React.Key (e.g., SelectOption<T extends React.Key =
string>) and update any related component generics/usage sites to respect the
new constraint; if you must support object values later, add a getKey: (value:
T) => React.Key and optional equals: (a: T, b: T) => boolean instead of allowing
arbitrary T.


export interface SelectUIComponents {
Expand All @@ -22,10 +22,11 @@ export interface SelectUIComponents {
ChevronIcon?: React.ComponentType<React.SVGProps<SVGSVGElement>>;
}

export interface SelectProps extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'value' | 'onChange'> {
options: SelectOption[];
value?: string;
onValueChange?: (value: string) => void;
export interface SelectProps<T = string>
extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'value' | 'onChange'> {
options: SelectOption<T>[];
value?: T;
onValueChange?: (value: T) => void;
Comment on lines +25 to +29
Copy link

@coderabbitai coderabbitai bot Aug 25, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Mirror the React.Key constraint on SelectProps; optionally firm up controlled vs. uncontrolled API

  • Essential: Propagate the React.Key constraint so options/value/onValueChange remain type-safe and compatible with DOM attributes and key usage.
  • Optional: The component behaves as controlled-only; consider typing the props as a controlled/uncontrolled union if you intend to support local state later.

Apply this diff:

-export interface SelectProps<T = string>
+export interface SelectProps<T extends React.Key = string>
   extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'value' | 'onChange'> {
   options: SelectOption<T>[];
-  value?: T;
-  onValueChange?: (value: T) => void;
+  value?: T;
+  onValueChange?: (value: T) => void;

Optional controlled/uncontrolled typing (outside this hunk) you can adopt later:

type Controlled<T extends React.Key = string> = {
  value: T;
  onValueChange: (value: T) => void;
};
type Uncontrolled<T extends React.Key = string> = {
  defaultValue?: T;
  onValueChange?: (value: T) => void;
};
export type SelectProps<T extends React.Key = string> =
  BaseButtonProps & { options: SelectOption<T>[] } & (Controlled<T> | Uncontrolled<T>);

To ensure downstream wrappers propagate the generic, especially remix-hook-form/select.tsx, run:

I can open a follow-up PR to generify the remix wrapper if needed.

🏁 Script executed:

#!/bin/bash
# Verify remix-hook-form wrapper uses the generic UISelectProps<T> and re-exports generic SelectProps<T>
fd -a 'select\.tsx$' packages/components/src/remix-hook-form | while read -r f; do
  echo "---- $f"
  rg -n -C2 'UISelectProps<|export interface SelectProps(.*)<|export function Select(.*)<' "$f"
done

Length of output: 1387

Propagate T extends React.Key through remix-hook-form wrappers

The remix-hook-form wrappers for Select are currently stripping out the new generic from UISelectProps<T>. To maintain type-safety and consistency with the core SelectProps<T extends React.Key>, we need to generify each wrapper.

• packages/components/src/remix-hook-form/select.tsx

  • Change the exported props interface to accept T extends React.Key = string and forward it to UISelectProps<T>
  • Update the Select component signature to <T extends React.Key = string>(props: SelectProps<T>) and render <UISelect<T> … />
-export interface SelectProps extends Omit<UISelectProps, 'value' | 'onValueChange'> {
+export interface SelectProps<T extends React.Key = string>
+  extends Omit<UISelectProps<T>, 'value' | 'onValueChange'> {
   name: string;
   label?: string;
}

-export function Select(props: SelectProps) {
+export function Select<T extends React.Key = string>(props: SelectProps<T>) {
   return <UISelect {...props} />;
}

• packages/components/src/remix-hook-form/canada-province-select.tsx
• packages/components/src/remix-hook-form/us-state-select.tsx

  • Both import UISelectProps without a generic parameter and export a non-generic component.
  • Introduce <T extends React.Key = string> on the props and component, and call <UISelect<T> … />.

Example diff for one wrapper:

-import { Select as UISelect, type SelectProps as UISelectProps } from '../ui/select';
-
-export interface CanadaProvinceSelectProps
-  extends Omit<UISelectProps, 'value' | 'onValueChange'> {}
-
-export function CanadaProvinceSelect(props: CanadaProvinceSelectProps) {
+import { Select as UISelect, type SelectProps as UISelectProps } from '../ui/select';
+
+export interface CanadaProvinceSelectProps<T extends React.Key = string>
+  extends Omit<UISelectProps<T>, 'value' | 'onValueChange'> {}
+
+export function CanadaProvinceSelect<T extends React.Key = string>(
+  props: CanadaProvinceSelectProps<T>
+) {
    return <UISelect options={canadaProvinceOptions} {...props} />;
}

Essential: Apply the same generic <T extends React.Key> pattern across all remix-hook-form wrappers so they correctly propagate the generic constraint.
Optional: Later, if you’d like to support both controlled and uncontrolled usage, consider adopting the controlled/uncontrolled union pattern discussed previously.

🤖 Prompt for AI Agents 
In packages/components/src/ui/select.tsx around lines 25 to 29, the SelectProps
generic is currently declared as SelectProps<T = string> but the remix-hook-form
wrappers expect T extends React.Key; update this file and the three
remix-hook-form wrappers to use the constrained generic. Specifically: change
the exported SelectProps declaration to SelectProps<T extends React.Key =
string>, then update any Select component signatures to <T extends React.Key =
string>(props: SelectProps<T>) and ensure any internal or exported UISelect
usage is parameterized as UISelect<T>. Apply the same change in
packages/components/src/remix-hook-form/select.tsx,
packages/components/src/remix-hook-form/canada-province-select.tsx, and
packages/components/src/remix-hook-form/us-state-select.tsx — make their
exported props interfaces generic as SelectProps<T extends React.Key = string>,
change the component definitions to accept the generic (<T extends React.Key =
string>(props: SelectProps<T>)), and render the core component as <UISelect<T>
... /> so the generic propagates through.

placeholder?: string;
disabled?: boolean;
className?: string;
Expand All @@ -34,7 +35,7 @@ export interface SelectProps extends Omit<React.ButtonHTMLAttributes<HTMLButtonE
components?: Partial<SelectUIComponents>;
}

export function Select({
export function Select<T = string>({
options,
value,
onValueChange,
Expand All @@ -45,7 +46,7 @@ export function Select({
itemClassName,
components,
...buttonProps
}: SelectProps) {
}: SelectProps<T>) {
const popoverState = useOverlayTriggerState({});
const listboxId = React.useId();
const [query, setQuery] = React.useState('');
Expand Down Expand Up @@ -174,7 +175,7 @@ export function Select({
const isSelected = option.value === value;
const isEnterCandidate = query.trim() !== '' && enterCandidate?.value === option.value && !isSelected;
return (
<li key={option.value} className="list-none">
<li key={String(option.value)} className="list-none">
Copy link

@coderabbitai coderabbitai bot Aug 25, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use the actual value as the React key (no String cast) once T is React.Key

Casting to String masks duplicate keys for non-primitives. With T constrained to React.Key, prefer the raw value for stable, unique keys.

Apply this diff:

-                <li key={String(option.value)} className="list-none">
+                <li key={option.value} className="list-none">

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In packages/components/src/ui/select.tsx around line 178, the list item
currently uses String(option.value) as the React key which masks duplicate keys
for non-primitive values; update the component so the generic type T is
constrained to React.Key (e.g., T extends React.Key) and use the raw
option.value directly as the key (key={option.value}) instead of casting to
String, ensuring the key prop receives the actual React.Key value for stable
uniqueness.

<Item
ref={isSelected ? selectedItemRef : undefined}
onClick={() => {
Expand Down