Enhance Select component value type #135
Conversation
|
|
WalkthroughSelect component API in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant S as Select<T>
participant CB as onValueChange(value: T)
U->>S: Select option (value: T)
S->>S: Update selected value (T)
S-->>CB: Call onValueChange with T
Note over S,CB: DOM keys use String(option.value)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
lcmohsen
changed the title
…or improved type flexibility.
lcmohsen
force-pushed
the
fix/make-select-options-generic
branch
from
b673ab9 to
eda3b38
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/src/ui/select.tsx (1)
38-49: Propagate the React.Key constraint to the component signatureAlign the component generic with the constrained props to prevent unsafely wide instantiation and fix key typing downstream.
Apply this diff:
-export function Select<T = string>({ +export function Select<T extends React.Key = string>({ options, value, onValueChange, placeholder = 'Select an option', disabled = false, className, contentClassName, itemClassName, components, ...buttonProps -}: SelectProps<T>) { +}: SelectProps<T>) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/components/package.json(2 hunks)packages/components/src/ui/select.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🧰 Additional context used
📓 Path-based instructions (10)
packages/components/src/ui/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration
Files:
packages/components/src/ui/select.tsx
packages/components/src/ui/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Base UI components should be named as ComponentName in ui/ directory
Files:
packages/components/src/ui/select.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema
Files:
packages/components/src/ui/select.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Always export both the component and its props type
Files:
packages/components/src/ui/select.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)
Files:
packages/components/src/ui/select.tsx
{apps,packages}/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')
Files:
packages/components/src/ui/select.tsx
packages/components/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports
Files:
packages/components/src/ui/select.tsx
{apps,packages}/**/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)
Files:
packages/components/src/ui/select.tsx
packages/components/src/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Don't import from remix-hook-form package in ui components
Files:
packages/components/src/ui/select.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Avoid circular dependencies
Files:
packages/components/src/ui/select.tsx
🧬 Code graph analysis (1)
packages/components/src/ui/select.tsx (1)
packages/components/src/remix-hook-form/select.tsx (2)
SelectProps(7-20)Select(22-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
| export interface SelectOption<T = string> { | ||
| label: string; | ||
| value: string; | ||
| value: T; | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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 SelectProps<T = string> | ||
| extends Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'value' | 'onChange'> { | ||
| options: SelectOption<T>[]; | ||
| value?: T; | ||
| onValueChange?: (value: T) => void; |
There was a problem hiding this comment.
💡 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"
doneLength 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 = stringand forward it toUISelectProps<T> - Update the
Selectcomponent 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
UISelectPropswithout 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.
| 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"> |
There was a problem hiding this comment.
🛠️ 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.
The
SelectOptionandSelectPropsandSelectaccept a type to be set as thevaluetype. (defaults tostring).Summary by CodeRabbit
New Features
Refactor
Documentation
Bug Fixes