refactor: rename remix form components to form components #30
Conversation
|
|
WalkthroughThis pull request removes a set of legacy Remix component files and their aggregated export, which previously centralized various form-related components. The individual components—such as checkboxes, date pickers, text fields, text areas, and forms—have been deleted from the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Remix Hook Form Component (e.g., Checkbox)
participant H as useRemixFormContext Hook
participant B as Base UI Component
U->>C: Interact (e.g., click/type)
C->>H: Request control context
H-->>C: Return control object
C->>B: Render base UI component with control and props
B-->>C: Render output
C-->>U: Display updated UI
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/src/form/form-checkbox.tsx (1)
1-18: Well-structured form component implementation.The component follows a clean pattern that integrates with Remix form context while hiding implementation details from consumers.
However, there's a small inconsistency in prop ordering compared to other similar components:
- return <Checkbox control={control} {...props} components={components} />; + return <Checkbox control={control} components={components} {...props} />;Spreading props after explicit props is generally safer to ensure explicit props aren't accidentally overridden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/components/src/form/form-checkbox.tsx(1 hunks)packages/components/src/form/form-date-picker.tsx(1 hunks)packages/components/src/form/form-dropdown-menu-select.tsx(1 hunks)packages/components/src/form/form-otp-input.tsx(1 hunks)packages/components/src/form/form-radio-group.tsx(1 hunks)packages/components/src/form/form-switch.tsx(1 hunks)packages/components/src/form/form-text-field.tsx(1 hunks)packages/components/src/form/form-textarea.tsx(1 hunks)packages/components/src/form/index.ts(1 hunks)packages/components/src/remix/index.ts(0 hunks)packages/components/src/remix/remix-checkbox.tsx(0 hunks)packages/components/src/remix/remix-form.tsx(0 hunks)packages/components/src/remix/remix-text-field.tsx(0 hunks)packages/components/src/remix/remix-textarea.tsx(0 hunks)
💤 Files with no reviewable changes (5)
- packages/components/src/remix/index.ts
- packages/components/src/remix/remix-textarea.tsx
- packages/components/src/remix/remix-checkbox.tsx
- packages/components/src/remix/remix-form.tsx
- packages/components/src/remix/remix-text-field.tsx
✅ Files skipped from review due to trivial changes (5)
- packages/components/src/form/form-date-picker.tsx
- packages/components/src/form/form-dropdown-menu-select.tsx
- packages/components/src/form/form-radio-group.tsx
- packages/components/src/form/index.ts
- packages/components/src/form/form-otp-input.tsx
🔇 Additional comments (5)
packages/components/src/form/form-textarea.tsx (1)
1-18: LGTM! Implementation follows consistent pattern.This component correctly follows the form component pattern established in this PR:
- Obtains control from Remix form context
- Sets up form-related sub-components
- Renders the base component with those elements
The props are ordered properly with spreading after explicit props, which prevents accidental overrides.
packages/components/src/form/form-text-field.tsx (1)
1-18: LGTM! Clean implementation with proper prop spreading.This component follows the same pattern as FormTextarea and maintains consistent prop ordering (spreading props after explicit props).
packages/components/src/form/form-switch.tsx (3)
4-6: Appropriate import update.The change from remix-form imports to local UI form imports is consistent with the overall refactoring goal.
6-10: Good interface renaming from Remix to Form prefix.The renaming aligns with the PR objectives to standardize component naming conventions.
12-20: Consistent component renaming and implementation.The function has been properly renamed from RemixSwitch to FormSwitch, and the internal components object now uses the correctly imported components from the UI library.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/docs/src/form/form-checkbox-list.stories.tsx(4 hunks)apps/docs/src/form/form-checkbox.stories.tsx(5 hunks)apps/docs/src/form/form-date-picker.stories.tsx(4 hunks)apps/docs/src/form/form-dropdown-menu-select.stories.tsx(5 hunks)apps/docs/src/form/form-otp-input.stories.tsx(4 hunks)apps/docs/src/form/form-radio-group.stories.tsx(5 hunks)apps/docs/src/form/form-switch.stories.tsx(4 hunks)apps/docs/src/form/form-text-field.stories.tsx(4 hunks)apps/docs/src/form/form-textarea.stories.tsx(4 hunks)packages/components/src/form/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/src/form/index.ts
🧰 Additional context used
🪛 GitHub Actions: Run Tests
apps/docs/src/form/form-text-field.stories.tsx
[error] 329-329: StorybookTestRunnerError: Found a label with the text of: Username, however no form control was found associated to that label. Make sure you're using the 'for' attribute or 'aria-labelledby' attribute correctly.
apps/docs/src/form/form-checkbox-list.stories.tsx
[error] 329-329: StorybookTestRunnerError: Found a label with the text of: Red, however no form control was found associated to that label. Make sure you're using the 'for' attribute or 'aria-labelledby' attribute correctly.
apps/docs/src/form/form-checkbox.stories.tsx
[error] 329-329: StorybookTestRunnerError: Found a label with the text of: Accept terms and conditions, however no form control was found associated to that label. Make sure you're using the 'for' attribute or 'aria-labelledby' attribute correctly.
🔇 Additional comments (17)
apps/docs/src/form/form-textarea.stories.tsx (3)
2-2: Component import updated to match naming convention.The import has been correctly updated from
RemixTextareatoFormTextareato align with the standardized naming convention for form components.
39-39: Component usage updated to use FormTextarea.The component reference has been correctly updated to use the new
FormTextareacomponent.
77-80: Storybook metadata updated to reference FormTextarea.The Storybook configuration has been correctly updated to reference the new
FormTextareacomponent in both the title, component reference, and type assertion.Also applies to: 90-90
apps/docs/src/form/form-switch.stories.tsx (3)
2-2: Component import updated to match naming convention.The import has been correctly updated from
RemixSwitchtoFormSwitchto align with the standardized naming convention for form components.
39-40: Component usage updated to use FormSwitch.Both switch instances have been correctly updated to use the new
FormSwitchcomponent.
61-64: Storybook metadata updated to reference FormSwitch.The Storybook configuration has been correctly updated to reference the new
FormSwitchcomponent in both the title, component reference, and type assertion.Also applies to: 74-74
apps/docs/src/form/form-text-field.stories.tsx (3)
2-2: Component import updated to match naming convention.The import has been correctly updated from
RemixTextFieldtoFormTextFieldto align with the standardized naming convention for form components.
40-40: Component usage updated to use FormTextField.The component reference has been correctly updated to use the new
FormTextFieldcomponent.
78-80: Storybook metadata updated to reference FormTextField.The Storybook configuration has been correctly updated to reference the new
FormTextFieldcomponent in both the title, component reference, and type assertion.Also applies to: 96-96
apps/docs/src/form/form-checkbox.stories.tsx (4)
2-2: Component import updated to match naming convention.The import has been correctly updated from
RemixCheckboxtoFormCheckboxto align with the standardized naming convention for form components.
41-41: Component usage updated to use FormCheckbox.All checkbox instances have been correctly updated to use the new
FormCheckboxcomponent.Also applies to: 42-47, 48-48
69-72: Storybook metadata updated to reference FormCheckbox.The Storybook configuration has been correctly updated to reference the new
FormCheckboxcomponent in both the title, component reference, and type assertion.Also applies to: 82-82
145-147: Example code in documentation updated.The documentation example has been updated to use
FormCheckboxconsistently.apps/docs/src/form/form-date-picker.stories.tsx (1)
2-2: LGTM! Component rename is consistent across the file.The component has been consistently renamed from
RemixDatePickertoFormDatePickeracross all instances in the file, including import, component usage, and metadata references.Also applies to: 18-18, 35-35, 60-62, 68-68, 73-73
apps/docs/src/form/form-radio-group.stories.tsx (1)
2-2: LGTM! Component rename is consistent across the file.The component has been consistently renamed from
RemixRadioGroupFieldtoFormRadioGroupFieldacross all instances in the file, including import, component usage, and metadata references.Also applies to: 21-21, 38-56, 80-82, 88-88, 93-93
apps/docs/src/form/form-otp-input.stories.tsx (1)
2-2: LGTM! Component rename is consistent across the file.The component has been consistently renamed from
RemixOTPInputFieldtoFormOTPInputFieldacross all instances in the file, including import, component usage, and metadata references.Also applies to: 19-19, 36-36, 66-68, 74-74, 79-79
apps/docs/src/form/form-dropdown-menu-select.stories.tsx (1)
2-2: Good job with consistent renaming!The component has been renamed from
RemixDropdownMenuSelecttoFormDropdownMenuSelectconsistently throughout the file. All references including imports, component declaration, JSX usage, Storybook metadata, and type references have been properly updated while maintaining the existing functionality.Also applies to: 21-21, 38-47, 72-75, 80-80, 85-85
| <RemixFormProvider {...methods}> | ||
| <fetcher.Form onSubmit={methods.handleSubmit}> | ||
| <RemixTextField name="username" label="Username" description="Enter a unique username" /> | ||
| <FormTextField name="username" label="Username" description="Enter a unique username" /> |
There was a problem hiding this comment.
Fix accessibility issue with FormTextField label.
The pipeline tests are failing because the test can't find a form control associated with the "Username" label.
This suggests that the implementation of FormTextField might have an issue with label association. Make sure the component correctly uses the htmlFor attribute or aria-labelledby to associate the label with the input field.
#!/bin/bash
# Description: Check the implementation of FormTextField
cat packages/components/src/form/form-text-field.tsxThere was a problem hiding this comment.
But we are using the base Form components from ShadCn? Do you think we may need to make separate form components to make sure the labels are hooked up correctly?
There was a problem hiding this comment.
🧩 Analysis chain
@Bfunk54 Let me check the implementation details of the form components to understand the issue better. The failing tests suggest there might be an accessibility issue with how the labels are associated with the form controls after the refactoring.
🏁 Script executed:
#!/bin/bash
# First, let's examine the FormTextField component
cat packages/components/src/form/form-text-field.tsx
# Let's also look at the base Form components from ShadCn
# assuming they might be in the ui/form directory since that's the common pattern
cat packages/components/src/ui/form.tsx 2>/dev/null || echo "File not found"
# Let's check if there's a difference in how the old RemixTextField was implemented
if [ -f packages/components/src/remix/remix-text-field.tsx ]; then
echo "--- Old RemixTextField implementation ---"
cat packages/components/src/remix/remix-text-field.tsx
fi
Length of output: 7129
🏁 Script executed:
#!/bin/bash
# Let's examine the TextField component to see how it uses the FormControl and FormLabel
cat packages/components/src/ui/text-field.tsx
# Let's also look at the tests to see how they're trying to find the form elements
cat apps/docs/src/form/form-text-field.stories.tsx
Length of output: 7089
@Bfunk54 After examining the components, I don't think we need to make separate form components. The issue is likely with how the tests are finding the elements after the refactoring.
The ShadCn Form components should be handling accessibility correctly - they use proper context, set the htmlFor attributes on labels, and associate them with inputs through IDs.
Looking at your tests, they're finding elements by:
- Getting the label text
- Finding the closest
.form-itemcontainer - Querying for the input within that container
This strategy might be failing if the DOM structure has changed slightly in the refactoring from RemixTextField to FormTextField.
I'd suggest trying these approaches:
- Add a
data-testidattribute to your input fields for more reliable selection in tests - Check if the class name
.form-itemis still being applied to the container - Try using more robust selectors like
canvas.getByLabelText('Username')which rely on proper label-input association
The ShadCn base components should be accessibility-compliant as long as we're passing the right props down through the component tree.
There was a problem hiding this comment.
Okay, the tests do work after refactoring so we should be good, thank you!
There was a problem hiding this comment.
@Bfunk54 Great! I'm glad to hear the tests are working after the refactoring. It seems like the component structure maintained proper accessibility relationships during the rename from RemixTextField to FormTextField. Let me know if you need any other assistance with the PR!
| <FormCheckbox className="rounded-md border p-4" name="terms" label="Accept terms and conditions" /> | ||
| <FormCheckbox | ||
| className="rounded-md border p-4" | ||
| name="marketing" | ||
| label="Receive marketing emails" | ||
| description="We will send you hourly updates about our products" | ||
| /> | ||
| <RemixCheckbox className="rounded-md border p-4" name="required" label="This is a required checkbox" /> | ||
| <FormCheckbox className="rounded-md border p-4" name="required" label="This is a required checkbox" /> |
There was a problem hiding this comment.
Fix accessibility issue with FormCheckbox label.
The pipeline tests are failing because the test can't find a form control associated with the "Accept terms and conditions" label.
This suggests that the implementation of FormCheckbox might have an issue with label association. Make sure the component correctly uses the htmlFor attribute or aria-labelledby to associate the label with the checkbox input.
#!/bin/bash
# Description: Check the implementation of FormCheckbox
cat packages/components/src/form/form-checkbox.tsx| @@ -1,5 +1,5 @@ | |||
| import { zodResolver } from '@hookform/resolvers/zod'; | |||
| import { RemixCheckbox } from '@lambdacurry/forms/remix/remix-checkbox'; | |||
| import { FormCheckbox } from '@lambdacurry/forms/form/form-checkbox'; | |||
There was a problem hiding this comment.
❓ Verification inconclusive
The component rename is causing test failures.
While the component has been consistently renamed from RemixCheckbox to FormCheckbox across all instances, the pipeline shows test failures related to form control association with labels. The test is looking for a form control (checkbox) associated with the label "Red" but cannot find it.
Also applies to: 70-70, 103-105, 116-116
🏁 Script executed:
#!/bin/bash
# Description: Check the implementation of FormCheckbox to see if it properly associates labels with checkboxes
# Check the FormCheckbox implementation
ast-grep --pattern $'function FormCheckbox({ label, name, description, className, ...props }: FormCheckboxProps) {
$$$
}'Length of output: 124
Attention: Verify Checkbox Label Association Implementation
The tests indicate that the renamed FormCheckbox component isn't associating the checkbox control with its label as expected (e.g., the "Red" label). Although the renaming from RemixCheckbox to FormCheckbox is consistent, our automated search did not produce output confirming that the component correctly links the label and checkbox (typically via matching htmlFor/id attributes).
Please manually verify that the FormCheckbox implementation (likely in @lambdacurry/forms/form/form-checkbox) properly connects the checkbox input with its corresponding label. Also, double-check the instances in:
apps/docs/src/form/form-checkbox-list.stories.tsxat lines 2, 70, 103–105, and 116
Ensuring that the markup meets the expected accessibility and test requirements should address the failing tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
packages/components/src/remix-hook-form/text-field.tsx (2)
5-5: Consider adding JSDoc comments for exported types.Adding documentation comments for the exported type would improve developer experience when using this component.
+/** + * Props for the TextField component, excluding the 'control' prop which is provided by the form context. + */ export type TextFieldProps = Omit<BaseTextFieldProps, 'control'>;
10-15: Consider extracting the components object to avoid recreation on each render.The
componentsobject is recreated on each render. For better performance, consider moving it outside the component or memoizing it.+const formComponents = { + FormControl, + FormLabel, + FormDescription, + FormMessage, +}; export function TextField(props: TextFieldProps) { const { control } = useRemixFormContext(); - const components = { - FormControl: FormControl, - FormLabel: FormLabel, - FormDescription: FormDescription, - FormMessage: FormMessage, - }; - return <BaseTextField control={control} components={components} {...props} />; + return <BaseTextField control={control} components={formComponents} {...props} />; }packages/components/src/remix-hook-form/date-picker.tsx (1)
4-4: Consider renaming to maintain consistent naming conventions.For consistency with other components, consider renaming to either:
DatePickerFieldPropsto maintain consistency withRadioGroupFieldProps- Rename the component to just
DatePickerif that aligns better with the overall naming strategypackages/components/src/remix-hook-form/textarea.tsx (2)
5-5: Consider naming consistency with other components.The component is named
Textareabut wraps aBaseTextareaField. For consistency, consider either:
- Rename to
TextareaFieldto match the base component name pattern- Ensure all other components follow a similar naming pattern (without 'Field' suffix)
10-15: Consider extracting the components object to avoid recreation on each render.Similar to the TextField component, the
componentsobject is recreated on each render. Consider moving it outside the component or memoizing it.+const formComponents = { + FormControl, + FormLabel, + FormDescription, + FormMessage, +}; export function Textarea(props: TextareaProps) { const { control } = useRemixFormContext(); - const components = { - FormControl: FormControl, - FormLabel: FormLabel, - FormDescription: FormDescription, - FormMessage: FormMessage, - }; - return <BaseTextareaField control={control} components={components} {...props} />; + return <BaseTextareaField control={control} components={formComponents} {...props} />; }packages/components/src/remix-hook-form/checkbox.tsx (1)
7-18: Consider optimizing components object creation.The components object is recreated on every render. While this doesn't significantly impact performance for most use cases, it could be optimized by moving it outside the component or using useMemo.
+const formComponents = { + FormDescription, + FormControl, + FormLabel, + FormMessage, +}; export function Checkbox(props: CheckboxProps) { const { control } = useRemixFormContext(); - const components = { - FormDescription: FormDescription, - FormControl: FormControl, - FormLabel: FormLabel, - FormMessage: FormMessage, - }; - return <BaseCheckbox control={control} {...props} components={components} />; + return <BaseCheckbox control={control} {...props} components={formComponents} />;packages/components/src/remix-hook-form/otp-input.tsx (1)
10-12: Improve error message specificity.The error message could be more specific to help developers understand which component is requiring the name property.
- throw new Error('Name is required for Input'); + throw new Error('Name is required for OTPInputField');packages/components/src/remix-hook-form/switch.tsx (1)
12-32: Consider optimizing components object creation.Similar to the Checkbox component, the components object is recreated on every render. This could be optimized by moving it outside the component function.
+const formComponents = { + FormDescription, + FormControl, + FormLabel, + FormMessage, +}; export function Switch({ name, label, description, className, ...props }: SwitchProps) { const { control } = useRemixFormContext(); - const components = { - FormDescription: FormDescription, - FormControl: FormControl, - FormLabel: FormLabel, - FormMessage: FormMessage, - }; return ( <BaseSwitchField control={control} name={name} label={label} description={description} - components={components} + components={formComponents} {...props} /> ); }apps/docs/src/remix-hook-form/checkbox.stories.tsx (1)
26-29: Type casting is still necessary but could be improved.The current approach of using
as truetype casting is maintained, but consider using a more type-safe pattern in the future.Instead of using type assertions, consider defining the schema with the correct types:
- terms: false as true, + terms: false, - required: false as true, + required: false,And adjust the schema definition to properly handle the initial false state:
const formSchema = z.object({ terms: z.boolean().refine((val) => val === true, 'You must accept the terms and conditions'), marketing: z.boolean().optional(), required: z.boolean().refine((val) => val === true, 'This field is required'), }).partial().refine(...); // Use partial to allow initial false stateapps/docs/src/remix-hook-form/text-field.stories.tsx (1)
103-105: Test selector approach improved.The selector approach has been updated to use a more robust method of finding elements by looking for the text label and then finding the closest form item container.
Consider extracting this selector pattern into a helper function since it's used multiple times throughout the tests:
+const getInputByLabel = (canvas, labelText) => { + const container = canvas.getByText(labelText).closest('.form-item'); + return container?.querySelector('input'); +}; const testDefaultValues = ({ canvas }: StoryContext) => { - const container = canvas.getByText('Username').closest('.form-item'); - const input = container?.querySelector('input'); + const input = getInputByLabel(canvas, 'Username'); expect(input).toHaveValue(INITIAL_USERNAME); };apps/docs/src/remix-hook-form/otp-input.stories.tsx (1)
66-68: Slight naming inconsistency in Storybook metadataWhile the component is named
OTPInputField, the story title usesFormOTPInputField. Consider updating either the component name or the story title for complete consistency.- title: 'Form/FormOTPInputField', + title: 'Form/OTPInputField',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/docs/src/remix-hook-form/checkbox-list.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/checkbox.stories.tsx(7 hunks)apps/docs/src/remix-hook-form/date-picker.stories.tsx(4 hunks)apps/docs/src/remix-hook-form/dropdown-menu-select.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/otp-input.stories.tsx(4 hunks)apps/docs/src/remix-hook-form/radio-group.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/switch.stories.tsx(4 hunks)apps/docs/src/remix-hook-form/text-field.stories.tsx(4 hunks)apps/docs/src/remix-hook-form/textarea.stories.tsx(4 hunks)packages/components/src/index.ts(0 hunks)packages/components/src/remix-hook-form/checkbox.tsx(1 hunks)packages/components/src/remix-hook-form/date-picker.tsx(1 hunks)packages/components/src/remix-hook-form/dropdown-menu-select.tsx(1 hunks)packages/components/src/remix-hook-form/index.ts(1 hunks)packages/components/src/remix-hook-form/otp-input.tsx(1 hunks)packages/components/src/remix-hook-form/radio-group.tsx(1 hunks)packages/components/src/remix-hook-form/switch.tsx(1 hunks)packages/components/src/remix-hook-form/text-field.tsx(1 hunks)packages/components/src/remix-hook-form/textarea.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/components/src/remix-hook-form/index.ts
🔇 Additional comments (61)
packages/components/src/remix-hook-form/text-field.tsx (1)
1-18: Well-structured component with proper context integration.The implementation of
TextFieldis clean and follows good patterns:
- It properly uses
useRemixFormContextto obtain the form control- The type definition correctly omits the 'control' property that will be provided by context
- Appropriate form components are passed through the components object
packages/components/src/remix-hook-form/date-picker.tsx (1)
1-10: Good implementation of the DatePicker component.The component properly integrates with the form context and follows a consistent pattern with other form components. The type definition is correctly defined to omit the 'control' property.
packages/components/src/remix-hook-form/radio-group.tsx (2)
1-10: Good implementation of the RadioGroupField component.The implementation is clean and properly integrates with the form context. The type is correctly defined to omit the 'control' property.
6-10:❓ Verification inconclusive
Verify if components object is needed for RadioGroupField.
Similar to the DatePicker component, this component doesn't pass a
componentsobject to the base component. IfBaseRadioGroupFieldaccepts form components, consider adding them for consistency with TextField and Textarea.
🏁 Script executed:
#!/bin/bash # Check if BaseRadioGroupField accepts a components prop grep -A 10 "export.*BaseRadioGroupField" packages/components/src/ui/radio-group-field.tsxLength of output: 91
Action Required: Manually Verify the Presence of a
componentsProp in BaseRadioGroupFieldAfter running the initial shell script, no evidence was found that
BaseRadioGroupFieldaccepts acomponentsprop. This outcome is inconclusive, so please perform a manual verification of its interface/type definition. If the base component is designed to support acomponentsprop (as seen with similar components likeTextFieldandTextarea), updateRadioGroupFieldaccordingly to pass thecomponentsobject for consistency.
- Verify manually if the
componentsprop exists in the declaration and usage ofBaseRadioGroupFieldinpackages/components/src/ui/radio-group-field.tsx.- If present, update
RadioGroupFieldto includecomponentswhen passing props toBaseRadioGroupField.- If absent, no changes are necessary.
packages/components/src/remix-hook-form/textarea.tsx (1)
1-18: Well-structured Textarea component with proper context integration.The implementation follows a good pattern consistent with the TextField component:
- It properly uses
useRemixFormContextto obtain the form control- The type definition correctly omits the 'control' property
- Appropriate form components are passed through the components object
packages/components/src/remix-hook-form/dropdown-menu-select.tsx (3)
1-3: LGTM: Clean imports with appropriate dependencies.The imports are well-structured, bringing in exactly what's needed from external dependencies and local UI components.
4-4: Well-defined type using Omit pattern.The type definition correctly omits the 'control' property from the base props since it will be provided internally via the context hook.
6-10: LGTM: Clean implementation of adapter pattern.The component follows a clean adapter pattern, getting the form control from context and passing it to the base component. This approach provides a consistent abstraction over the form library implementation.
packages/components/src/remix-hook-form/checkbox.tsx (2)
1-4: Clean imports with appropriate dependencies.The imports are well-structured, bringing in the remix form hook, base component, and form UI components.
5-5: Well-defined type using Omit pattern.The type definition correctly omits the 'control' property from the base props, following the same pattern as other form components.
packages/components/src/remix-hook-form/otp-input.tsx (4)
1-3: LGTM: Clean imports with appropriate dependencies.The imports are well-structured and follow the same pattern as other form components.
4-4: Well-defined type using Omit pattern.The type definition correctly omits the 'control' property from the base props, maintaining consistency with other form components.
6-9: LGTM: Good prop extraction and control access.The component correctly extracts required props and accesses the form control from context.
14-15: LGTM: Clean return statement with proper prop spreading.The component correctly passes all required props to the base component.
packages/components/src/remix-hook-form/switch.tsx (2)
1-5: LGTM: Complete imports with React types.The imports are comprehensive, including React types for proper TypeScript support along with all necessary components.
6-10: Well-structured interface with explicit props.The interface definition is thorough, extending the base component props and explicitly defining additional required and optional props. This improves type safety and documentation.
apps/docs/src/remix-hook-form/textarea.stories.tsx (4)
2-2: Import path updated correctly.The import statement has been properly updated to use the new path structure, maintaining consistency with the refactoring approach.
39-39: Component renamed properly from RemixTextarea to Textarea.The component usage has been correctly updated to use the new
Textareacomponent name, which aligns with the refactoring goal of removing the "Remix" prefix.
77-79: Meta configuration updated consistently.The Storybook metadata has been correctly updated to reflect the new component name and category. The change from 'Remix/RemixTextarea' to 'Form/Textarea' provides a more intuitive organization of components in the Storybook navigation.
90-90: Type definition updated correctly.The type satisfaction for the meta object has been properly updated to use the new component name.
apps/docs/src/remix-hook-form/checkbox.stories.tsx (5)
2-2: Import path updated correctly.The import path has been updated appropriately to use the new component from the refactored location.
41-57: Component renamed consistently throughout the component usage.All instances of the checkbox component have been updated to use the new name, maintaining consistency throughout the file.
77-90: Meta configuration updated appropriately.The Storybook metadata has been correctly updated to reflect the new component name and path structure.
96-102: Test selector approach updated to accommodate the new component structure.The test selectors have been updated to find checkboxes through their labels and closest
.form-itemcontainers, which is a robust approach that works with the new component structure.
113-124: Test logic improved with null checks.The test logic for submitting the form has been enhanced with proper null checks, making the tests more robust.
apps/docs/src/remix-hook-form/radio-group.stories.tsx (5)
2-2: Import path updated correctly.The import path has been properly updated to use the new component from its refactored location.
21-21: Function renamed to match component naming convention.The example function has been renamed from
RemixRadioGroupExampletoRadioGroupExamplefor consistency with the new component naming scheme.
38-56: Component usage updated consistently.The component has been properly renamed from
RemixRadioGroupFieldtoRadioGroupFieldthroughout its usage in the return statement.
80-93: Meta configuration updated appropriately.The Storybook metadata has been properly updated to reflect the new component name and the title has been changed to 'Form/FormRadioGroup'.
88-88: Component reference in decorator updated.The component reference in the decorator has been correctly updated to use the new component name.
apps/docs/src/remix-hook-form/switch.stories.tsx (5)
2-2: Import path updated correctly.The import path has been properly updated to use the new component from the refactored location.
39-41: Component usage updated consistently.The component has been correctly renamed from
RemixSwitchtoSwitchin all instances.
61-74: Meta configuration updated appropriately.The Storybook metadata has been correctly updated to reflect the new component name and organization.
79-85: Test selector logic updated for the new component structure.The test selectors have been updated to find switches through their labels and closest
.form-itemcontainers, making the tests more resilient.
87-101: Toggle test logic improved with null checks.The test logic for toggling switches has been enhanced with proper null checks, improving the robustness of the tests.
apps/docs/src/remix-hook-form/checkbox-list.stories.tsx (5)
2-2: Import path updated to reflect component reorganization.The import path has been updated to reference the component from its new location in the remix-hook-form directory, which aligns with the PR objective of renaming and reorganizing form components.
70-70: Component renamed from RemixCheckbox to Checkbox.The Checkbox component is now used without the "Remix" prefix, which is consistent with the PR's goal of removing legacy Remix component names.
103-105: Storybook metadata updated to reflect renamed component.The Storybook configuration has been appropriately updated to reference the new component name and title path, maintaining consistency across the documentation.
Also applies to: 116-116
123-125: Test selector approach improved.The selector approach has been updated to use a more robust method of finding elements by looking for the text label and then finding the closest form item container.
142-154: Test logic updated to accommodate new component structure.The test logic has been well-adapted to work with the new component structure, using more reliable selectors to target checkbox elements within their containers.
apps/docs/src/remix-hook-form/date-picker.stories.tsx (5)
2-2: Import path updated to new component location.The import statement has been updated to reference the DatePicker component from its new location in the remix-hook-form directory.
18-18: Component name updated to remove Remix prefix.The component name has been updated from RemixDatePickerExample to DatePickerExample, maintaining consistency with the PR's renaming objectives.
35-35: Component usage updated to use renamed component.The DatePicker component is now used without the "Remix" prefix in the JSX markup.
60-63: Storybook metadata updated for the renamed component.The Storybook configuration has been properly updated to reference the new component name and title, ensuring documentation remains consistent with the code changes.
Also applies to: 73-73
68-69: Component reference updated in decorators.The component reference in the decorators section has been properly updated to use the new component name.
apps/docs/src/remix-hook-form/dropdown-menu-select.stories.tsx (5)
2-2: Import path updated to reflect the new component location.The import statement has been updated to reference the DropdownMenuSelect component from its new location in the remix-hook-form directory.
21-21: Component name updated to remove Remix prefix.The component name has been updated from RemixDropdownMenuSelectExample to DropdownMenuSelectExample, aligning with the PR's renaming strategy.
38-47: Component usage updated with the renamed component.The DropdownMenuSelect component is now used without the "Remix" prefix in the JSX markup, while maintaining the same functionality and props structure.
72-74: Storybook metadata updated for the renamed component.The Storybook configuration has been appropriately updated to reference the new component name and title path.
Also applies to: 85-85
80-81: Component reference updated in decorators.The component reference in the decorators section has been properly updated to use the new component name.
apps/docs/src/remix-hook-form/text-field.stories.tsx (6)
2-2: Import path updated to reflect the new component location.The import statement has been updated to reference the TextField component from its new location in the remix-hook-form directory.
40-40: Component usage updated with the renamed component.The TextField component is now used without the "Remix" prefix in the JSX markup, while maintaining the same functionality.
78-81: Storybook metadata updated for the renamed component.The Storybook configuration has been appropriately updated to reference the new component name and title path.
Also applies to: 96-96
109-120: Test logic improved for invalid submission testing.The test logic has been enhanced with better error handling and a more robust approach to element selection. The additional comment about clicking the input before clearing is a helpful explanation.
126-139: Test logic improved with better async handling.Switching from
getByTexttofindByTextfor the error message check is a good improvement as it handles async updates more reliably.
142-152: Test logic improved for form submission.The test has been updated with proper null checks and a more resilient way to interact with form elements.
apps/docs/src/remix-hook-form/otp-input.stories.tsx (5)
2-2: Import path updated correctlyThe import statement has been properly updated to reflect the new component location and name, which aligns with the refactoring objective of removing Remix prefix from components.
19-19: Component name updated for clarityThe component has been renamed from
RemixOTPInputExampletoOTPInputFieldExample, which is consistent with the refactoring goal of removing the "Remix" prefix from component names.
36-41: Component reference updated consistentlyThe JSX has been updated to use the new component name
OTPInputFieldwhile maintaining the same props and behavior, ensuring backward compatibility during the refactoring.
74-74: Decorator component reference updatedThe decorator component reference has been correctly updated to use the new component name.
79-79: Type annotation updatedThe type annotation in the
satisfiesclause has been properly updated to reference the new component type.
| export function DatePicker(props: DatePickerProps) { | ||
| const { control } = useRemixFormContext(); | ||
|
|
||
| return <BaseDatePickerField control={control} {...props} />; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if components object is needed for DatePicker.
I notice that unlike TextField and Textarea, this component doesn't pass a components object to the base component. If BaseDatePickerField accepts form components similar to other base components, consider adding them for consistency.
🏁 Script executed:
#!/bin/bash
# Check if BaseDatePickerField accepts a components prop
grep -A 10 "export.*BaseDatePickerField" packages/components/src/ui/date-picker-field.tsxLength of output: 91
I'll generate a new shell script to search for any references to a components property within the BaseDatePickerField implementation to verify if it indeed accepts such a prop.
🏁 Script executed:
#!/bin/bash
# Search for any occurrence of "components" in the BaseDatePickerField file
rg "components" -n packages/components/src/ui/date-picker-field.tsxLength of output: 605
Attention: Our verification confirms that BaseDatePickerField does accept a components prop (e.g., see lines 31 and 35 in packages/components/src/ui/date-picker-field.tsx). For consistency with components like TextField and Textarea, please consider explicitly passing a default or merged components object from DatePicker rather than relying solely on the forwarded props. This adjustment will help ensure uniform customization across all form fields.
There was a problem hiding this comment.
@Bfunk54 You're welcome! I'm happy to have been able to help. Let me know if you need any assistance with implementing the components object for the DatePicker component or if you have any other questions about the refactoring process.
|
|
||
| export type RadioGroupFieldProps = Omit<BaseRadioGroupFieldProps, 'control'>; | ||
|
|
||
| export function RadioGroupField(props: RadioGroupFieldProps) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify naming convention consistency across components.
There appears to be inconsistency in the naming convention across the form components:
- This component is named
RadioGroupField - The text field component is named
TextField(without 'Field' suffix) - The textarea component is named
Textarea(without 'Field' suffix)
Consider standardizing the naming convention across all components for better developer experience.
🏁 Script executed:
#!/bin/bash
# Check the export names of all form components to verify naming consistency
grep -r "export function " packages/components/src/remix-hook-form/ | sortLength of output: 1001
Inconsistent Naming of Form Components – Standardize the 'Field' Suffix
After verifying the export names, there is an inconsistency in how the components are named:
-
Components without the “Field” suffix:
CheckboxDatePickerDropdownMenuSelectSwitchTextFieldTextarea
-
Components with the “Field” suffix:
OTPInputFieldRadioGroupField
To ensure a uniform developer experience, please consider aligning these names. Either remove the "Field" suffix from OTPInputField and RadioGroupField to match the majority, or add the suffix to the other components. This standardization will improve consistency across the form components.
There was a problem hiding this comment.
@Bfunk54 You're welcome! I'm glad I could help identify the naming inconsistency. Feel free to reach out if you need any further assistance with standardizing the component names or any other aspects of your form components refactoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/src/remix-hook-form/otp-input.tsx (1)
10-12: Update error message for clarityThe error message should reference the component name for better debugging.
- throw new Error('Name is required for Input'); + throw new Error('Name is required for OTPInput');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/src/remix-hook-form/otp-input.stories.tsx(4 hunks)apps/docs/src/remix-hook-form/radio-group.stories.tsx(5 hunks)packages/components/src/remix-hook-form/otp-input.tsx(1 hunks)packages/components/src/remix-hook-form/radio-group.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/src/remix-hook-form/radio-group.tsx
- apps/docs/src/remix-hook-form/otp-input.stories.tsx
- apps/docs/src/remix-hook-form/radio-group.stories.tsx
🔇 Additional comments (4)
packages/components/src/remix-hook-form/otp-input.tsx (4)
1-2: Good implementation of form field integrationThe imports are appropriately structured, importing the form context hook and the base OTP input field component.
4-4: Well-designed type definitionGood practice to use
Omit<>to derive the props type from the base component while removing thecontrolproperty that will be handled internally.
6-8: Clean integration with form contextThe component correctly uses the
useRemixFormContexthook to access the form control and properly destructures the props.
14-15: Properly structured component renderingThe component correctly passes the control from context, the name prop, and spreads the remaining props to the base component.
|
Does this look good @jaruesink ? |
| title: 'Remix/RemixCheckboxList', | ||
| component: RemixCheckbox, | ||
| const meta: Meta<typeof Checkbox> = { | ||
| title: 'Form/CheckboxList', |
There was a problem hiding this comment.
@Bfunk54 in the documentation we should actually probably nest these under RemixHookForm/CheckboxList
| AVAILABLE_COLORS.forEach(({ label }) => { | ||
| const checkbox = canvas.getByLabelText(label); | ||
| expect(checkbox).not.toBeChecked(); | ||
| const container = canvas.getByText(label).closest('.form-item'); |
There was a problem hiding this comment.
The form component was the reason. I have since fixed this, thank you.
| expect(termsCheckbox).not.toBeChecked(); | ||
| expect(marketingCheckbox).not.toBeChecked(); | ||
| expect(requiredCheckbox).not.toBeChecked(); | ||
| const termsContainer = canvas.getByText('Accept terms and conditions').closest('.form-item'); |
| expect(notificationsSwitch).not.toBeChecked(); | ||
| expect(darkModeSwitch).not.toBeChecked(); | ||
| const testDefaultValues = ({ canvas }: StoryContext) => { | ||
| const notificationsContainer = canvas.getByText('Enable notifications').closest('.form-item'); |
There was a problem hiding this comment.
why did these tests need to change?
|
|
||
| const testUsernameTaken = async ({ canvas }: StoryContext) => { | ||
| const input = canvas.getByLabelText('Username'); | ||
| const container = canvas.getByText('Username').closest('.form-item'); |
There was a problem hiding this comment.
why did these tests need to change?
There was a problem hiding this comment.
I don't think we want to have these barrel files to force people to use the tree shaken direct imports from the files
There was a problem hiding this comment.
Thank you! Makes sense, the barrel file is now removed.
| @@ -23,9 +23,9 @@ const ControlledCheckboxExample = () => { | |||
| const methods = useRemixForm<FormData>({ | |||
| resolver: zodResolver(formSchema), | |||
| defaultValues: { | |||
| terms: false as true, // Note: ZOD Schema expects a true value | |||
There was a problem hiding this comment.
put my comments back in please
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/src/remix-hook-form/form.tsx (2)
7-13: Consider removing redundantregisterproperty definition.The
FormProviderPropsinterface extendsUseFormReturn<T>with customizedhandleSubmitandresetmethods, which is good. However, theregisterproperty appears to be redundant as it's already included in theUseFormReturn<T>interface and doesn't seem to have a different type signature.export interface FormProviderProps<T extends FieldValues> extends Omit<UseFormReturn<T>, 'handleSubmit' | 'reset'> { children: React.ReactNode; handleSubmit: (e?: React.BaseSyntheticEvent) => Promise<void>; - register: UseFormRegister<T>; reset: (values?: T, keepStateOptions?: KeepStateOptions) => void; }
69-76: Consider enhancing error message handling in FormMessage.The
FormMessagecomponent correctly passes the error message from the form field context. However, it might be worth adding a fallback whenerrorexists but doesn't have amessageproperty.export const FormMessage = React.forwardRef< HTMLParagraphElement, React.ComponentPropsWithoutRef<typeof BaseFormMessage> >((props, ref) => { const { error, formMessageId } = useFormField(); - return <BaseFormMessage ref={ref} formMessageId={formMessageId} error={error?.message} {...props} />; + return <BaseFormMessage ref={ref} formMessageId={formMessageId} error={error?.message || (error ? String(error) : undefined)} {...props} />; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/docs/src/remix-hook-form/checkbox-list.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/checkbox.stories.tsx(6 hunks)apps/docs/src/remix-hook-form/date-picker.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/dropdown-menu-select.stories.tsx(6 hunks)apps/docs/src/remix-hook-form/otp-input.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/radio-group.stories.tsx(6 hunks)apps/docs/src/remix-hook-form/switch.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/text-field.stories.tsx(5 hunks)apps/docs/src/remix-hook-form/textarea.stories.tsx(5 hunks)packages/components/src/remix-hook-form/checkbox.tsx(1 hunks)packages/components/src/remix-hook-form/form.tsx(1 hunks)packages/components/src/remix-hook-form/switch.tsx(1 hunks)packages/components/src/remix-hook-form/text-field.tsx(1 hunks)packages/components/src/remix-hook-form/textarea.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/components/src/remix-hook-form/switch.tsx
- packages/components/src/remix-hook-form/checkbox.tsx
- apps/docs/src/remix-hook-form/switch.stories.tsx
- apps/docs/src/remix-hook-form/checkbox.stories.tsx
- packages/components/src/remix-hook-form/text-field.tsx
- apps/docs/src/remix-hook-form/radio-group.stories.tsx
- apps/docs/src/remix-hook-form/text-field.stories.tsx
- apps/docs/src/remix-hook-form/checkbox-list.stories.tsx
- apps/docs/src/remix-hook-form/dropdown-menu-select.stories.tsx
- apps/docs/src/remix-hook-form/textarea.stories.tsx
- packages/components/src/remix-hook-form/textarea.tsx
- apps/docs/src/remix-hook-form/otp-input.stories.tsx
- apps/docs/src/remix-hook-form/date-picker.stories.tsx
🔇 Additional comments (6)
packages/components/src/remix-hook-form/form.tsx (6)
1-5: Clear imports with good commenting practice.The use of the biome-ignore comment to prevent linting errors when exporting as a component library shows good attention to potential build issues. The import structure is clean and organized, separating core React imports from form-related dependencies.
38-41: Clean implementation of FormLabel component.The
FormLabelcomponent is implemented correctly with proper ref forwarding and display name setting for improved debugging experience.
43-58: FormControl component correctly handles error states.The component properly leverages the
useFormFieldhook to access error state and form identifiers, then passes these to the base component. The!!errorconversion to boolean is a good practice for ensuring consistent types.
60-67: FormDescription component is well structured.This component correctly forwards the ref and description ID from the form field context to the base component.
1-76:✅ Verification successful
Verify FormField component exists for context provision.
The
useFormFieldhook references a<FormField>component and expects to be used within it. However, this component isn't defined in the current file. Make sure there's a correspondingFormFieldcomponent that provides the required context.
🏁 Script executed:
#!/bin/bash # Description: Look for FormField component implementation that should provide the context needed by useFormField # Search for FormField component definition rg -g "*.tsx" -g "!*.test.tsx" -g "!*.stories.tsx" "export const FormField" --after-context=10Length of output: 1509
Verified: FormField component exists and provides the required context.
The
<FormField>component is implemented inpackages/components/src/ui/form.tsx(as confirmed by the search results) and supplies the necessary context for theuseFormFieldhook. No changes are needed in this file.
15-36:❓ Verification inconclusive
Robust hook implementation with proper context validation.
The
useFormFieldhook is well-implemented with appropriate error handling when used outside aFormFieldcomponent. It correctly extracts and combines field state with context identifiers.One thing to verify is whether the imported
BaseFormFieldContextis exported from the form component that contains the corresponding provider.
🏁 Script executed:
#!/bin/bash # Description: Check if BaseFormFieldContext is properly exported from ui/form # and if there's a FormField component that provides this context # Check the form.tsx import source cat packages/components/src/ui/form.tsx 2>/dev/null || echo "File not found" # Look for FormField component that might provide the context rg -A 5 "FormFieldContext.Provider" --glob "*.tsx"Length of output: 6838
Attention: Verify Context Naming Consistency
The
useFormFieldhook implementation is robust and includes proper error handling. However, please double-check that theBaseFormFieldContextimported in this file indeed maps to the context provided by theFormFieldcomponent. Our verification confirmed that inpackages/components/src/ui/form.tsx, the context is exported asFormFieldContext. IfBaseFormFieldContextis intended to be an alias forFormFieldContext, please ensure that the alias is correctly set up to avoid any confusion.
- Confirm that
BaseFormFieldContextis correctly exported or re-exported from the source that providesFormFieldContextin the UI form module.- Consider renaming for clarity if these contexts refer to the same source.
|
Okay, so the latest few commits should have addressed all concerns. Barrel file has been removed, all tests were put back to the original, some were failing due to the form.tsx component not being set up correctly, I've now ensured it's setup correctly. I've also addressed the Meta titles. @jaruesink |
…thanks to coderabbit
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/src/remix-hook-form/form.tsx (1)
21-23: Add informative error message.Consider expanding the error message to be more informative for developers who might misuse this hook. For example, specify where FormField is exported from.
if (!fieldContext) { - throw new Error('useFormField should be used within <FormField>'); + throw new Error('useFormField should be used within <FormField> component from "../ui/form"'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/src/remix-hook-form/form.tsx(1 hunks)
🔇 Additional comments (5)
packages/components/src/remix-hook-form/form.tsx (5)
42-57: Handling of form control error prop looks good.The FormControl component correctly maps the field error state to the expected props for the BaseFormControl component. The use of the double bang operator (
!!error) to convert the error object to a boolean is a good pattern.
68-74: Good error message handling with fallbacks.The FormMessage component handles error messages well by first checking for error?.message, then falling back to String(error) if there's an error without a message property, and finally defaulting to undefined if there's no error. This is a robust approach to handling different error formats.
1-75: Consistent naming convention aligns with PR objectives.The component naming (FormLabel, FormControl, etc.) without the "Remix" prefix aligns with the PR objective of renaming Remix form components to more generalized form components. This improves consistency and clarity in the codebase.
7-12:✅ Verification successful
Interface design looks good but validates handleSubmit properly.
The interface correctly extends and omits parts of
UseFormReturnwhile adding custom properties. This is a good design pattern for customizing existing types.However, ensure that the custom
handleSubmitmatches the expected behavior of react-hook-form's version, especially regarding the return type and error handling.
🏁 Script executed:
#!/bin/bash # Check how handleSubmit is typically implemented with react-hook-form echo "Checking for handleSubmit implementations..." rg -p "handleSubmit.*=>.*Promise<void>" --type tsLength of output: 267
Interface Design is Solid — Verified Implementation of
handleSubmitThe custom
handleSubmitsignature, defined as(e?: React.BaseSyntheticEvent) => Promise<void>, has been confirmed to match the expected pattern from react-hook-form. The interface correctly extendsUseFormReturn(omittinghandleSubmitandreset) while adding sensible custom types.
- Verification: The rg search confirmed the signature exists as intended.
- Recommendation: Ensure that the underlying implementation of
handleSubmitconsistently handles errors in line with react-hook-form’s behavior.
14-35:✅ Verification successful
Check the FormField component reference.
The useFormField hook throws an error if not used within a
<FormField>component, but I don't see a FormField component defined in this file. Make sure the FormField component is defined elsewhere and properly exported for use with this hook.
🏁 Script executed:
#!/bin/bash # Look for the FormField component definition echo "Looking for FormField component..." rg -A 3 "export.*FormField" --type tsxLength of output: 147
Below is a new shell script to re-run the search—this one explicitly adds TSX file support so we can better locate any exported
FormFielddefinitions:After running this script, if the output still does not show a definition for
FormField, please manually verify whether the<FormField>component is defined and exported in another file. TheuseFormFieldhook relies on this component (and throws an error if not used within<FormField>). It is important to ensure that the component exists and is correctly exported for consistent usage of the hook.Action: Please validate that the
FormFieldcomponent is indeed defined somewhere in the codebase and properly exported for use with theuseFormFieldhook (located inpackages/components/src/remix-hook-form/form.tsxat lines 14-35).
🏁 Script executed:
#!/bin/bash # Add TSX file type support and search for an exported FormField component rg --type-add "tsx:*.tsx" -g "*.tsx" -A 3 "export\s+(const|function|class)\s+FormField"Length of output: 807
Verified: FormField Component Reference
The search confirms that the
FormFieldcomponent is defined and exported frompackages/components/src/ui/form.tsx. TheuseFormFieldhook’s requirement to be used within<FormField>is valid. No further changes are needed; please ensure that when using the hook, the correct<FormField>component from that file is imported.
| import { useRemixFormContext } from 'remix-hook-form'; | ||
| import { FormControl as BaseFormControl, FormDescription as BaseFormDescription, FormFieldContext as BaseFormFieldContext, FormItemContext as BaseFormItemContext, FormLabel as BaseFormLabel, FormMessage as BaseFormMessage } from '../ui/form'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the import paths and dependencies.
The imports from 'remix-hook-form' and '../ui/form' suggest this file is part of a refactoring from Remix-specific components to more general form components. Ensure that these dependencies exist and are correctly set up in your project structure.
🏁 Script executed:
#!/bin/bash
# Check if the imported modules exist
# Check for remix-hook-form package
echo "Checking for remix-hook-form package..."
if [ -d "node_modules/remix-hook-form" ] || [ -d "packages/remix-hook-form" ]; then
echo "remix-hook-form found"
else
# Look for local modules with this name
echo "Looking for local remix-hook-form modules..."
fd -t f "index.(ts|tsx|js|jsx)" -p | xargs grep -l "export.*useRemixFormContext"
fi
# Check for the ui/form components
echo "Checking for ui/form components..."
fd -t f "form.(ts|tsx)" -p "packages/components/src/ui"Length of output: 521
Review Feedback: Validate and Correct Dependency Paths in packages/components/src/remix-hook-form/form.tsx
- The import from
'../ui/form'is valid—the corresponding form components were found inpackages/components/src/ui/form.tsx. - However, the dependency for
remix-hook-form(providinguseRemixFormContext) could not be located in eithernode_modulesor a local package directory. Please verify that this package is correctly installed or that the import path is correct. If it's meant to be a local module, ensure it is added to the repository as expected.
There was a problem hiding this comment.
This can't be right, every component uses remix-hook-form. It is installed.
There was a problem hiding this comment.
🧩 Analysis chain
@Bfunk54 You're absolutely right. The remix-hook-form package is indeed installed and used across multiple components in your codebase.
I found numerous imports of remix-hook-form in several TypeScript and React files, including multiple components in the packages/components/src/remix-hook-form/ directory. The package is being used correctly for the useRemixFormContext hook that you're using in the form components.
It seems my previous verification script wasn't comprehensive enough to detect the package. This is likely because:
- The package might be a dependency specified in a workspace-level or root-level package.json
- The node_modules structure in a monorepo can be complex, and my initial search wasn't thorough enough
My apologies for the confusion. The imports in your form.tsx file are correct, and the refactoring you're doing to rename the components appears to be on the right track.
🏁 Script executed:
#!/bin/bash
# Check for remix-hook-form in package.json files
echo "Checking for remix-hook-form in package.json files..."
fd package.json | xargs grep -l "remix-hook-form"
# Check for actual imports of remix-hook-form in the codebase
echo -e "\nChecking for imports of remix-hook-form in the codebase..."
rg "from ['\"]remix-hook-form['\"]" --type ts --type tsx
# Look for all imports from remix-hook-form package
echo -e "\nLooking for all imports from remix-hook-form package..."
rg "import.*from ['\"]remix-hook-form['\"]" --type ts --type tsx
# Check specific file that imports useRemixFormContext
echo -e "\nChecking the form.tsx file that imports useRemixFormContext..."
cat packages/components/src/remix-hook-form/form.tsx | grep -A 5 "import.*from 'remix-hook-form'"
# Check for other component files in the same directory to see if they have similar imports
echo -e "\nChecking other component files in the same directory..."
find packages/components/src/remix-hook-form -name "*.tsx" -type f | xargs grep -l "import.*from 'remix-hook-form'"
Length of output: 2086
@Bfunk54 You're absolutely right, and I apologize for the confusion. The script results clearly confirm that remix-hook-form is properly installed and being used throughout your codebase:
- It's listed as a dependency in
packages/components/package.json - The
useRemixFormContextimport in yourform.tsxfile is correct:import { useRemixFormContext } from 'remix-hook-form';
- All of your component files in the
packages/components/src/remix-hook-form/directory properly import from this package
My previous verification was incorrect. Your PR refactoring the component names appears to be on the right track, with all the proper dependencies in place. Thanks for pointing this out.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/src/remix-hook-form/form.tsx (1)
21-23: Consider adding a more descriptive error messageThe error message is accurate but could be more helpful by suggesting what the user needs to do.
- throw new Error('useFormField should be used within <FormField>'); + throw new Error('useFormField must be used within a <FormField> component. Ensure this hook is called from a component that is a child of FormField.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/src/remix-hook-form/form.tsx(1 hunks)
🔇 Additional comments (8)
packages/components/src/remix-hook-form/form.tsx (8)
3-5: Verify the import paths and dependenciesThis is a new file implementing forms functionality which imports from two key locations:
react-hook-form- A standard npm libraryremix-hook-form- Custom package that needs to be validated../ui/form- Local UI componentsBased on the past review comment, ensure that
remix-hook-formexists in your project. The previous verification script couldn't locate this dependency.#!/bin/bash # Check if the imported useRemixFormContext exists echo "Checking for remix-hook-form dependency..." # Check if it's an npm dependency if [ -f "package.json" ]; then echo "Checking package.json for remix-hook-form..." grep -A 3 "dependencies" package.json | grep "remix-hook-form" fi # Look for local modules with this name echo "Looking for local useRemixFormContext definition..." fd -t f "index.(ts|tsx)" -p "packages" | xargs grep -l "export.*useRemixFormContext" echo "Checking for related files in the remix-hook-form directory..." fd -t f -e ts -e tsx -p "packages/components/src/remix-hook-form"
7-12: FormProviderProps interface looks goodThe interface extends
UseFormReturnfrom react-hook-form, appropriately omitting and redefining thehandleSubmitandresetmethods to match the needs of your form implementation. The use of generics withT extends FieldValuesprovides good type flexibility.
14-35: Well-implemented hook with proper error handlingThe
useFormFieldhook correctly:
- Retrieves context from both form field and item contexts
- Uses
useRemixFormContextto access form state- Includes appropriate error handling when used outside a FormField component
- Returns a well-structured object with all necessary field information
This implementation follows React best practices for custom hooks.
37-40: FormLabel component implementation looks goodThe component correctly uses React.forwardRef to pass refs to the base component, ensuring proper component composition. Setting the displayName is also a good practice for debugging.
42-57: FormControl correctly integrates field stateThe component properly leverages the useFormField hook to extract error state and relevant IDs, passing them to the base component. The use of !!error to convert to boolean is a good practice for ensuring consistent error state types.
59-66: FormDescription implementation looks goodThis component follows the same pattern as the other form components, correctly using the useFormField hook to access the required context data.
68-75: Robust error handling in FormMessageThe FormMessage component handles different types of error values through the expression
error?.message || (error ? String(error) : undefined), which accommodates both structured error objects and primitive error values.This is a good defensive programming approach that prevents rendering issues with different error formats.
1-75:❓ Verification inconclusive
Verify this component fits in the broader refactoring strategy
This file is part of renaming "remix form components" to "form components" as mentioned in the PR description. The implementation looks good, but ensure that it's properly referenced wherever the previous components were used, and that any tests have been updated accordingly.
The PR comment mentioned "all tests were reverted to their original state" and that previously "some tests were failing due to the form.tsx component not being set up correctly" - confirm that this implementation addresses those issues.
🏁 Script executed:
#!/bin/bash # Check for references to both old and new form components echo "Checking for references to old Remix form components..." rg -l "RemixForm(Label|Control|Description|Message)" --type ts --type tsx echo "Checking for references to new form components..." rg -l "FormProvider|useFormField|Form(Label|Control|Description|Message)" --type ts --type tsx | grep -v "remix-hook-form/form.tsx" echo "Checking test files for form components..." fd -t f -p "test|spec" | grep -E "\.(tsx|ts|js|jsx)$" | xargs grep -l "Form"Length of output: 665
I'll need to further verify that all references to the old Remix form component names have been replaced with the new ones and that the tests now reference the updated components correctly. It appears our initial search commands ran into issues with TSX file types, so please run the following updated shell script in your repository to confirm that:
#!/bin/bash # Define TSX file type for ripgrep searches and check both old and new form component references echo "Checking for references to old Remix form components..." rg --type-add 'tsx:*.tsx' -l 'RemixForm(Label|Control|Description|Message)' . echo "Checking for references to new form components..." rg --type-add 'tsx:*.tsx' -l 'FormProvider|useFormField|Form(Label|Control|Description|Message)' . echo "Checking test files for usage of form components..." find . -type f \( -iname "*test*.tsx" -o -iname "*spec*.tsx" -o -iname "*test*.ts" -o -iname "*spec*.ts" -o -iname "*test*.jsx" -o -iname "*spec*.jsx" -o -iname "*test*.js" -o -iname "*spec*.js" \) \ -exec grep -l "Form" {} +Action Required: Verify that:
- There are no lingering references to the old
RemixForm…component names.- All imports, references, and tests now use the new naming convention (e.g.
FormProvider,useFormField,FormLabel,FormControl,FormDescription, andFormMessage).- The issues previously observed in tests related to this file have been resolved.
Once you manually verify that the updated form components are correctly integrated and that tests pass, please update the review accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/components/src/remix-hook-form/form.tsx (5)
1-2: Ensure necessity of lint rule suppression.This comment (
// biome-ignore lint/style/noNamespaceImport) suggests that importing React via* as Reactis compelled by a specific lint rule scenario. If it’s no longer essential, consider removing the suppression to keep the file free from unnecessary overrides.
7-12: Add documentation for theFormProviderPropsif needed.This interface extends
UseFormReturnbut omits methods, presumably to create a custom form provider. If it’s part of your public API, consider adding JSDoc comments explaining its purpose to downstream consumers.
14-35: Guard against missingBaseFormItemContext.While
fieldContextis validated,itemContextis assumed to exist. IfFormItemContextmight be undefined, destructuring at line 25 would cause a runtime error. Either ensure it’s always provided at a higher level, or add a similar check before destructuring.For example:
const itemContext = React.useContext(BaseFormItemContext); if (!itemContext) { - const { id, formItemId, ... } = itemContext; + throw new Error('useFormField must be used within a <FormItem> component.'); }
42-57: Leverage typed error handling.Currently, error is handled as a general shape (
!!erroranderror?.message). If your codebase uses strongly typed error objects, consider narrower handling or transform them to clarify how errors are communicated.
68-75: Account for non-standard error objects.If
errormight be a string, object, or another structure, the fallbackString(error)is correct but can be made more explicit by clarifying the expected shape oferror. Otherwise, no major concerns here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/src/remix-hook-form/form.tsx(1 hunks)
🔇 Additional comments (3)
packages/components/src/remix-hook-form/form.tsx (3)
3-6: Imports appear consistent and well-organized.The use of
react-hook-form,remix-hook-form, and../ui/formreflect a coherent separation of concerns. No issues are detected here.
37-41: Clean usage of forwardRef.Allowing refs to be passed to
FormLabelis useful for accessibility testing and ensures consistent composition in third-party integrations. This approach is good to have.
59-67: Great structure forFormDescription.Using
useFormFieldto derive IDs ensures correct association with the parent form component. This fosters good accessibility and maintainability.
Summary by CodeRabbit
Refactor
New Features