[PROPOSAL] New React Hook Form Abtraction #639
Conversation
WalkthroughAdds a new form system built on react-hook-form: Form provider/wrapper, FormField context and Controller integration, field adapters (FieldText, FieldSelect), UI primitives (Field suite, Label, Select type export), storybook docs, small input styling tweak, new utilities, and two Radix dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as Form Component
participant Provider as FormProvider
participant FormField as FormField
participant Controller
participant FieldComp as Field Variant
participant Context as FormFieldContext
User->>Form: Render (onSubmit?, noHtmlForm flag)
Form->>Provider: Wrap children with FormProvider
Provider->>FormField: Render FormField(name, size, controller props)
FormField->>Controller: Initialize field binding
Controller->>FormField: Provide field + fieldState
FormField->>Context: Memoize and provide (field, fieldState, size)
FieldComp->>Context: useFormField() reads context
FieldComp->>FieldComp: Render input/select with aria attrs, id
User->>FieldComp: Input / select value
FieldComp->>Controller: Trigger onChange/onBlur -> update form state
Controller->>Provider: Update form state -> re-render affected FieldComp
FieldComp->>FieldComp: Render error UI when fieldState.invalid
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/types/utilities.d.ts (1)
39-42: Consider a more robust WithRequired implementation.The current implementation intersects with
{ [_ in TKey]: {} }, which doesn't preserve the original types of the required keys. The empty object type{}represents "any non-nullish value" in TypeScript, not an empty object.Consider using a standard approach that preserves type information:
-type WithRequired<TTarget, TKey extends keyof TTarget> = TTarget & { - // eslint-disable-next-line @typescript-eslint/no-empty-object-type - [_ in TKey]: {}; -}; +type WithRequired<TTarget, TKey extends keyof TTarget> = TTarget & + Required<Pick<TTarget, TKey>>;This ensures the required keys retain their original types from
TTarget.src/components/new-form/form-field/index.tsx (1)
56-66: Consider alternatives to useMemo in the render prop.While the current pattern works (and is acknowledged with the eslint-disable), calling
useMemoinside a render function is unconventional and flagged by static analysis. Consider these alternatives:
- Extract the render logic to a separate component that can use hooks at the top level
- Remove
useMemoentirely sincefieldandfieldStateare already stable references from ControllerExample of removing useMemo:
render={({ field, fieldState }) => { - // We are inside a render function so it's fine - // eslint-disable-next-line react-hooks/rules-of-hooks - const fieldCtx = useMemo( - () => ({ - field, - fieldState, - size, - }), - [field, fieldState, size] - ) as FormFieldContextValue; + const fieldCtx = { + field, + fieldState, + size, + } as FormFieldContextValue; return (src/components/ui/field.tsx (1)
127-138: Consider using a distinct data-slot for FieldTitle.Both
FieldTitle(line 130) andFieldLabel(line 115) usedata-slot="field-label". While this might be intentional for styling purposes, it could cause confusion when selecting elements with CSS or JavaScript.If they serve different semantic purposes, consider using
data-slot="field-title"for FieldTitle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
package.json(1 hunks)src/components/new-form/_field-components.ts(1 hunks)src/components/new-form/docs.stories.tsx(1 hunks)src/components/new-form/field-select/index.tsx(1 hunks)src/components/new-form/field-text/index.tsx(1 hunks)src/components/new-form/form-field-label.tsx(1 hunks)src/components/new-form/form-field/context.tsx(1 hunks)src/components/new-form/form-field/index.tsx(1 hunks)src/components/new-form/form.tsx(1 hunks)src/components/new-form/index.ts(1 hunks)src/components/ui/field.tsx(1 hunks)src/components/ui/input.tsx(1 hunks)src/components/ui/label.tsx(1 hunks)src/components/ui/select.tsx(1 hunks)src/lib/react-hook-form/index.tsx(1 hunks)src/types/utilities.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/components/new-form/index.ts (2)
src/components/form/docs.stories.tsx (2)
form(70-114)form(30-68)src/components/form/field-text/docs.stories.tsx (3)
form(55-82)form(31-53)form(84-112)
src/components/new-form/field-select/index.tsx (3)
src/components/ui/select.tsx (2)
TValueBase(23-23)Select(55-212)src/components/new-form/form-field/context.tsx (1)
useFormField(20-26)src/components/form/field-select/index.tsx (3)
TFieldValues(25-95)div(53-92)e(82-85)
src/components/new-form/_field-components.ts (4)
src/components/new-form/form-field-label.tsx (1)
FormFieldLabel(4-10)src/components/new-form/field-text/index.tsx (1)
FieldText(5-35)src/components/new-form/field-select/index.tsx (1)
FieldSelect(6-45)src/components/form/form-field-controller.tsx (1)
TFieldValues(72-140)
src/components/new-form/docs.stories.tsx (4)
src/components/new-form/form.tsx (1)
Form(25-54)src/lib/zod/zod-utils.ts (1)
zu(12-42)src/lib/react-hook-form/index.tsx (1)
useForm(26-37)src/components/form/docs.stories.tsx (3)
form(70-114)form(30-68)z(24-28)
src/components/new-form/field-text/index.tsx (2)
src/components/new-form/form-field/context.tsx (1)
useFormField(20-26)src/components/form/field-text/index.tsx (4)
e(71-74)div(52-82)TFieldValues(26-85)e(75-78)
src/components/new-form/form-field-label.tsx (2)
src/components/ui/field.tsx (1)
FieldLabel(242-242)src/components/new-form/form-field/context.tsx (1)
useFormField(20-26)
src/components/new-form/form-field/index.tsx (3)
src/components/new-form/form-field/context.tsx (3)
FormFieldSize(8-8)FormFieldContextValue(10-14)FormFieldContext(16-18)src/components/new-form/_field-components.ts (2)
FieldComponents(14-14)fieldComponents(5-12)src/components/ui/field.tsx (1)
Field(237-237)
src/components/new-form/form-field/context.tsx (3)
src/components/form/field-text/index.tsx (3)
TFieldValues(26-85)div(52-82)e(71-74)src/components/form/field-checkbox/index.tsx (1)
field(50-75)src/components/form/field-checkbox-group/index.tsx (1)
field(59-101)
src/lib/react-hook-form/index.tsx (2)
src/components/new-form/form-field/index.tsx (1)
FormField(44-82)src/components/new-form/index.ts (1)
FormField(4-4)
src/components/new-form/form.tsx (3)
src/components/form/form.tsx (1)
e(38-47)src/components/form/docs.stories.tsx (2)
form(70-114)form(30-68)src/components/form/form-test-utils.tsx (1)
T(14-41)
🪛 Biome (2.1.2)
src/components/new-form/docs.stories.tsx
[error] 44-44: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 54-54: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
src/components/new-form/form-field/index.tsx
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 GitHub Check: 🧹 Linter
src/components/ui/field.tsx
[warning] 214-214:
Do not use item index in the array as its key
⏰ 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). (2)
- GitHub Check: 🔬 Tests (lts/*)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (8)
src/components/ui/input.tsx (1)
13-13: LGTM! More precise ARIA invalid state handling.Narrowing the selector to
has-[[aria-invalid=true]]correctly ensures invalid styling only applies when aria-invalid is explicitly true, not when it's false or missing. This aligns well with the new form-field system's centralized error handling.src/components/new-form/form-field-label.tsx (1)
1-10: LGTM! Clean composition pattern.The component properly consumes form field context and wires up the label associations correctly. The
idandhtmlForattributes ensure proper accessibility linkage between label and input.package.json (1)
57-58: LGTM! Appropriate dependencies for accessible primitives.The Radix UI Label and Separator components provide solid accessible foundations for the new form field system.
src/components/ui/label.tsx (1)
1-22: LGTM! Well-structured accessible label component.The component properly wraps Radix UI's Label primitive with appropriate styling and disabled state handling. The use of both
group-data-[disabled=true]andpeer-disabledvariants ensures proper disabled styling in various composition scenarios.src/components/ui/select.tsx (1)
23-23: LGTM! Appropriate type export for form integration.Exporting
TValueBaseenables the new form field components to properly constrain their generic type parameters without duplicating the type definition.src/components/new-form/field-text/index.tsx (1)
5-35: Clean implementation with proper handler chaining.The component correctly:
- Integrates with form field context
- Chains onChange/onBlur handlers to preserve both RHF and custom behavior
- Conditionally renders errors based on validation state
- Maintains consistent ID patterns for accessibility
src/components/new-form/index.ts (1)
1-4: LGTM! Clean barrel export for the new form API.The exports properly consolidate the public API surface for the form abstraction.
src/components/ui/field.tsx (1)
1-247: Well-structured Field component library.The Field UI component library demonstrates solid design patterns:
- Consistent use of
data-slotattributes for composability- Clean separation of concerns with focused components
- Good use of
class-variance-authorityfor variant management- Comprehensive coverage of form field scenarios
The composable API will integrate well with the new FormField abstraction.
| aria-describedby={ | ||
| !fieldState.error ? `${descriptionId}` : `${descriptionId} ${errorId}` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify description element exists for aria-describedby.
The aria-describedby attribute always includes descriptionId, but it's unclear whether a description element with this ID is always rendered. If no description exists, this creates an invalid ARIA reference.
Run the following script to check how descriptions are handled in the form field context:
🏁 Script executed:
#!/bin/bash
# Search for description rendering in form field components
rg -n "desc\"|description" --type tsx -g 'src/components/new-form/**' -C3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Read the component file to understand aria-describedby and description rendering
cat src/components/new-form/field-text/index.tsxLength of output: 975
🏁 Script executed:
#!/bin/bash
# Search for description ID and description element rendering in form components
rg -n "descriptionId|description" --type ts -g 'src/components/new-form/**' -C2Length of output: 1644
🏁 Script executed:
#!/bin/bash
# Find and read the form-field context/wrapper component
find src/components/new-form -name "*form-field*" -type fLength of output: 44
🏁 Script executed:
#!/bin/bash
# Search for description element rendering - look for description ID usage and element creation
rg -n "desc.*id|<.*id.*desc" --type ts -g 'src/components/new-form/**' -A2 -B2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Read the form-field-label component
cat src/components/new-form/form-field-label.tsxLength of output: 339
🏁 Script executed:
#!/bin/bash
# List all files in new-form directory
find src/components/new-form -type f -name "*.tsx" -o -name "*.ts"Length of output: 380
🏁 Script executed:
#!/bin/bash
# Broader search for where descriptionId might be used or rendered
rg -n "\-desc|descriptionId" --type ts -C3Length of output: 15824
Add description element rendering or conditionally render aria-describedby.
The aria-describedby attribute always references descriptionId (line 16), but the component never renders an element with this ID. This creates an invalid ARIA reference. Either:
- Add a
<FieldDescription id={descriptionId}>element rendering in the component, or - Conditionally set
aria-describedbyonly when a description is actually rendered
The <FieldError> is already conditionally rendered when fieldState.invalid (line 27), but there's no corresponding description element for the non-error state.
🤖 Prompt for AI Agents
In src/components/new-form/field-text/index.tsx around lines 15 to 17,
aria-describedby always includes descriptionId even though no element with that
ID is rendered; either render a FieldDescription element with id={descriptionId}
when a description exists, or change aria-describedby so it only includes
descriptionId when that description is actually rendered (keep including errorId
when fieldState.error is present); update the JSX to conditionally render the
description element or conditionally build aria-describedby accordingly so no
invalid ARIA reference remains.
| const fieldCtx = useMemo( | ||
| () => ({ | ||
| field, | ||
| fieldState, | ||
| size, | ||
| }), | ||
| [field, fieldState] | ||
| ) as FormFieldContextValue; |
There was a problem hiding this comment.
Add 'size' to the useMemo dependency array.
The memoized fieldCtx includes size (line 63) but the dependency array on line 65 only includes [field, fieldState]. When size changes, the context will contain a stale value.
Apply this diff:
const fieldCtx = useMemo(
() => ({
field,
fieldState,
size,
}),
- [field, fieldState]
+ [field, fieldState, size]
) as FormFieldContextValue;📝 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.
| const fieldCtx = useMemo( | |
| () => ({ | |
| field, | |
| fieldState, | |
| size, | |
| }), | |
| [field, fieldState] | |
| ) as FormFieldContextValue; | |
| const fieldCtx = useMemo( | |
| () => ({ | |
| field, | |
| fieldState, | |
| size, | |
| }), | |
| [field, fieldState, size] | |
| ) as FormFieldContextValue; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 59-59: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/components/new-form/form-field/index.tsx around lines 59 to 66, the
useMemo that builds fieldCtx includes the value "size" but the dependency array
only lists [field, fieldState], so fieldCtx can become stale when size changes;
update the dependency array to include size (i.e., [field, fieldState, size]) so
the memo recomputes whenever size updates.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/ui/field.tsx (3)
213-219: Consider using error message as the key for better semantics.While the current implementation using
${id}-error-${index}is stable across remounts (thanks to theuseIdprefix), using the error message itself as the key would be more semantically correct since errors are deduplicated by message. This ensures React's reconciliation aligns with the logical identity of each error.Apply this diff:
<ul className="ml-4 flex list-disc flex-col gap-1"> {uniqueErrors.map( - (error, index) => + (error) => error?.message && ( - // eslint-disable-next-line @eslint-react/no-array-index-key - <li key={`${id}-error-${index}`}>{error.message}</li> + <li key={error.message}>{error.message}</li> ) )} </ul>Note: Since errors are deduplicated by message at line 203-204, each
error.messagein the final array should be unique, making it a suitable key.
203-204: Handle undefined error messages in deduplication logic.The Map-based deduplication will collapse all errors with
undefinedmessages into a single entry. If multiple validation rules fail without providing messages, only one undefined error will be preserved.Apply this diff to filter out undefined messages before deduplication:
const uniqueErrors = [ - ...new Map(errors.map((error) => [error?.message, error])).values(), + ...new Map( + errors + .filter((error) => error?.message) + .map((error) => [error.message, error]) + ).values(), ];
207-207: Use strict equality===instead of==.Loose equality can lead to unexpected type coercion.
Apply this diff:
- if (uniqueErrors?.length == 1) { + if (uniqueErrors?.length === 1) { return uniqueErrors[0]?.message; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/field.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T11:07:14.833Z
Learnt from: ivan-dalmet
Repo: BearStudio/start-ui-web PR: 532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to `useEffect` dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
Applied to files:
src/components/ui/field.tsx
⏰ 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: Playwright E2E Tests
Proposal
This PR introduces a new
FormFieldabstraction built on top of React Hook Form’sController, inspired by [TanStack Form].It replaces the current
FormFieldController(type-switch–based) with a render-props, composition-first API that’s easier to extend and customize.Note
At this stage we’re just aiming to validate the pattern.
a11y helpers,
...otherpassthroughs, and sugar components will follow.Why a new abstraction ?
The current
FormFieldControllerworks, but it becomes costly as our field catalog grows:FormFieldControllerand vice versa, making the module graph brittle.The new
FormFieldfocuses on composition and layering:{ props, state }and a registry of field components.fieldComponentsmap. No core edits.It’s also closer to industry-standard patterns (e.g., shadcn UI Field composables) and aligns with TanStack Form’s ergonomics, which keeps a future switch feasible.
Comparison
Pros & Cons
✅ Pros (Proposal)
Label/Description/Errorpieces).FormFieldController.Summary by CodeRabbit
New Features
Documentation
Chores
Style