feat: migrate to Biome v2.2.4 #143
Conversation
- Update @biomejs/biome from 1.9.4 to 2.2.4 - Migrate biome.json configuration to v2 format - Fix package script syntax from --apply to --write - Remove deprecated 'all': true rule syntax 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
|
|
🔍 Broken test auto-fixer • Learn more
|
- Install Playwright browsers and system dependencies for CI tests - Auto-fix import organization and formatting issues with Biome v2 - Resolve most linting warnings while preserving functionality - Tests now pass successfully with Biome v2.2.4 configuration
|
✅ Fixed failing check suites on PR #143! I've successfully resolved the issues that were causing the GitHub Actions workflows to fail: 🔧 Issues Fixed:
✅ Results:
The PR is now ready with a clean Biome v2.2.4 migration that maintains all existing functionality while updating to the latest tooling standards. 🔗 Updated commit: 1dbe46e 💻 View my work • 🛑 Stop • 🚫 Ban all checks • React 👍 or 👎 |
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
|
✅ Fixed all failing check suites on PR #143! 🔧 Issues Resolved1. Build Failures ✅
2. Test Failures ✅
3. Biome v2 Migration Validation ✅
📊 Current Status
🚀 Key Improvements
The Biome v2 migration is now complete and fully functional! The failing checks were due to missing CI dependencies, not the migration itself. All core functionality is working as expected. 💻 View my work • 🛑 Stop • 🚫 Ban all checks • React 👍 or 👎 |
|
@codegen-sh resolve the merge conflicts
|
…lect popover width) [skip ci]
|
@jaruesink Merge conflicts resolved ✅ What I did:
Pushed in commit 0bd9cd3: 0bd9cd3 CI checks are running now; I’ll follow up if anything needs adjustment. 💻 View my work • 🛑 Stop • React 👍 or 👎 |
- Renamed `format-and-lint` scripts to `lint` for clarity and consistency. - Adjusted Biome configuration to disable automatic import organization. - Updated AGENTS.md and package.json to reflect these changes. - Enhanced type definitions in various components for better type safety.
WalkthroughBumps Biome and updates lint scripts; adds a Researcher agent doc; tightens types across data-table filters and remix-hook-form wrappers; reorders many imports/exports; refactors Select UI with keyboard/list behaviors; introduces calendar WeekNumber override and OTP input signature change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Trigger as Select Trigger
participant Popover as Popover / Search
participant List as Options List
User->>Trigger: click
Trigger->>Popover: open()
Popover->>List: render(SearchInput + filtered options)
User->>Popover: type / ArrowUp / ArrowDown / Enter / Esc
Popover->>List: update activeIndex / selection
alt Enter on active
List->>Popover: select(item)
Popover->>Trigger: focus() and close()
Popover->>Popover: clear query
else Esc
Popover->>Popover: clear query
Popover->>Trigger: close()
end
sequenceDiagram
autonumber
participant Page
participant DTRF as DataTableRouterForm
participant Hook as useDataTableFilters
participant TB as DataTableRouterToolbar
participant Table as DataTable
Page->>DTRF: mount(props)
DTRF->>Hook: init(columnsConfig, options, faceted)
Hook-->>DTRF: {columns, actions, state}
DTRF->>TB: render(toolbar props)
TB->>Hook: dispatch(action)
Hook-->>DTRF: updated state
DTRF->>Table: render with updated state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
jaruesink
temporarily deployed
to
pr-preview
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (46)
packages/components/src/ui/calendar.tsx (2)
3-3: Type-only import of DayButton used withtypeoflater will break type-checking.
React.ComponentProps<typeof DayButton>requires a value symbol; importingDayButtonwithtypeerases the value. Use a props type derived from DayPicker’scomponentsinstead.Apply this diff:
-import { type DayButton, DayPicker } from 'react-day-picker'; +import { DayPicker } from 'react-day-picker';Then update the DayButton props typing as in the next comment.
107-113: Fix CalendarDayButton props typing to avoidtypeofon a type-only symbol.Derive the props from DayPicker’s components API; also export the props type per guidelines.
Apply this diff:
-function CalendarDayButton({ className, day, modifiers, ...props }: React.ComponentProps<typeof DayButton>) { +export type CalendarDayButtonProps = + Parameters<NonNullable<React.ComponentProps<typeof DayPicker>['components']>['DayButton']>[0]; + +function CalendarDayButton({ className, day, modifiers, ...props }: CalendarDayButtonProps) {packages/components/src/ui/data-table-filter/components/filter-selector.tsx (3)
108-111: Icon‑only trigger lacks an accessible name when filters are activeWhen
hasFiltersis true, the button renders only an icon. Add an aria-label so screen readers can announce the control.Apply this diff:
- <Button variant="outline" className={cn('h-7', hasFilters && 'w-fit !px-2')}> + <Button + variant="outline" + className={cn('h-7', hasFilters && 'w-fit !px-2')} + aria-label={t('filter', locale)} + >
21-27: Export props types per package guidelinesProps interfaces should be exported alongside the component for consumers.
Apply this diff:
-interface FilterSelectorProps<TData> { +export interface FilterSelectorProps<TData> {
195-202: Export props types per package guidelinesSame as above for QuickSearchFilters.
Apply this diff:
-interface QuickSearchFiltersProps<TData> { +export interface QuickSearchFiltersProps<TData> {packages/components/src/ui/data-table-filter/hooks/use-debounce-callback.tsx (1)
48-50: isPending always true; wrapper ignores inner pending stateYou’re returning truthiness of the ref, not the timer state. Delegate to the underlying debounced instance.
Apply this diff:
- wrappedFunc.isPending = () => { - return !!debouncedFunc.current; - }; + wrappedFunc.isPending = () => { + return debouncedFuncInstance.isPending(); + };Optionally also clear the ref on cancel:
- wrappedFunc.cancel = () => { - debouncedFuncInstance.cancel(); - }; + wrappedFunc.cancel = () => { + debouncedFuncInstance.cancel(); + debouncedFunc.current = null; + };packages/components/src/ui/data-table-filter/lib/debounce.ts (1)
39-49: Fix leading-edge invocation in shouldInvoke (leading: true broken)
shouldInvoke returns false on first call (lastCallTime === null), preventing immediate invocation when leading: true — apply the diff below.File: packages/components/src/ui/data-table-filter/lib/debounce.ts — apply:
- function shouldInvoke(time: number): boolean { - if (lastCallTime === null) return false; - const timeSinceLastCall = time - lastCallTime; - const timeSinceLastInvoke = time - lastInvokeTime; - return ( - lastCallTime === null || - timeSinceLastCall >= wait || - timeSinceLastCall < 0 || - (maxWaitTime !== null && timeSinceLastInvoke >= maxWaitTime) - ); - } + function shouldInvoke(time: number): boolean { + if (lastCallTime === null) return true; + const timeSinceLastCall = time - lastCallTime; + const timeSinceLastInvoke = time - lastInvokeTime; + return ( + timeSinceLastCall >= wait || + timeSinceLastCall < 0 || + (maxWaitTime !== null && timeSinceLastInvoke >= maxWaitTime) + ); + }
- Duplicate debounce implementation found at packages/components/src/ui/utils/debounce.ts — consider consolidating.
packages/components/src/ui/text-field.tsx (3)
15-26: Export a named props type for FieldPrefix (avoid inline prop typing).Repo guidelines require ComponentNameProps and exporting both the component and its props type. Define and export FieldPrefixProps, then use it in the signature.
+export interface FieldPrefixProps { + children: React.ReactNode + className?: string +} -export const FieldPrefix = ({ children, className }: { children: React.ReactNode; className?: string }) => { +export const FieldPrefix = ({ children, className }: FieldPrefixProps) => { return ( <div className={cn( 'flex h-full text-base items-center pl-3 pr-0 text-gray-500 group-focus-within:text-gray-700 transition-colors duration-200 border-y border-l border-input rounded-l-md bg-background', className, )} > <span className="whitespace-nowrap">{children}</span> </div> ); };
28-39: Export a named props type for FieldSuffix (avoid inline prop typing).Same rationale as FieldPrefix: add and export FieldSuffixProps and use it in the signature.
+export interface FieldSuffixProps { + children: React.ReactNode + className?: string +} -export const FieldSuffix = ({ children, className }: { children: React.ReactNode; className?: string }) => { +export const FieldSuffix = ({ children, className }: FieldSuffixProps) => { return ( <div className={cn( 'flex h-full text-base items-center pr-3 pl-0 text-gray-500 group-focus-within:text-gray-700 transition-colors duration-200 border-y border-r border-input rounded-r-md bg-background', className, )} > <span className="whitespace-nowrap">{children}</span> </div> ); };
1-1: Bug: overriding react-hook-form’s field.ref breaks registration; switch to forwardRef and compose refs.Setting ref={ref} after spreading {...field} overwrites field.ref, causing inputs not to register/focus correctly. Also, using React.forwardRef requires a value import of React (not type-only).
-import type * as React from 'react'; +import * as React from 'react'; @@ export const TextField = function TextField({ - control, - name, - label, - description, - className, - components, - prefix, - suffix, - ref, - ...props -}: TextInputProps & { ref?: React.Ref<HTMLInputElement> }) { + control, + name, + label, + description, + className, + components, + prefix, + suffix, + ...props +}: TextInputProps) { @@ - <InputComponent - {...field} - {...props} - ref={ref} + <InputComponent + {...field} + {...props} className={cn('focus-visible:ring-0 focus-visible:ring-offset-0 border-input', { 'rounded-l-none border-l-0': prefix, 'rounded-r-none border-r-0': suffix, })} /> @@ -}; +}; + +// Compose multiple refs (field.ref + forwarded ref) +const composeRefs = + <T,>(...refs: Array<React.Ref<T> | undefined>) => + (node: T | null) => { + for (const ref of refs) { + if (!ref) continue; + if (typeof ref === 'function') ref(node as T); + else (ref as React.MutableRefObject<T | null>).current = node; + } + }; + +export const TextField = React.forwardRef<HTMLInputElement, TextInputProps>(function TextField( + props, + ref, +) { + const { control, name, label, description, className, components, prefix, suffix, ...rest } = props; + const InputComponent = components?.Input || TextInput; + return ( + <FormField + control={control} + name={name} + render={({ field, fieldState }) => ( + <FormItem className={className}> + {label && <FormLabel Component={components?.FormLabel}>{label}</FormLabel>} + <div + className={cn('flex group transition-all duration-200 rounded-md', { + 'field__input--with-prefix': prefix, + 'field__input--with-suffix': suffix, + 'focus-within:ring-2 focus-within:ring-ring focus-within:ring-offset-2 focus-within:ring-offset-background': + true, + })} + > + {prefix && <FieldPrefix>{prefix}</FieldPrefix>} + <FormControl Component={components?.FormControl}> + <InputComponent + {...field} + {...rest} + ref={composeRefs<HTMLInputElement>(field.ref, ref)} + className={cn('focus-visible:ring-0 focus-visible:ring-offset-0 border-input', { + 'rounded-l-none border-l-0': prefix, + 'rounded-r-none border-r-0': suffix, + })} + /> + </FormControl> + {suffix && <FieldSuffix>{suffix}</FieldSuffix>} + </div> + {description && ( + <FormDescription Component={components?.FormDescription}>{description}</FormDescription> + )} + {fieldState.error && ( + <FormMessage Component={components?.FormMessage}>{fieldState.error.message}</FormMessage> + )} + </FormItem> + )} + /> + ); +}); + +TextField.displayName = 'TextField';Also applies to: 55-66, 88-91
apps/docs/src/remix-hook-form/radio-group-custom.stories.tsx (1)
176-179: Use fetcher.Form in stories per docs rules.Stories needing Router integration should wrap forms with
fetcher.Form.Apply this diff:
- <RemixFormProvider {...methods}> - <Form onSubmit={methods.handleSubmit}> + <RemixFormProvider {...methods}> + <fetcher.Form onSubmit={methods.handleSubmit}> ... - </Form> + </fetcher.Form>- <Button type="submit" className="mt-6 bg-purple-600 hover:bg-purple-700"> - Submit - </Button> - {fetcher.data?.message && <p className="mt-2 text-green-600">{fetcher.data.message}</p>} - </Form> + <Button type="submit" className="mt-6 bg-purple-600 hover:bg-purple-700"> + Submit + </Button> + {fetcher.data?.message && <p className="mt-2 text-green-600">{fetcher.data.message}</p>} + </fetcher.Form>Also applies to: 363-369
packages/components/src/remix-hook-form/use-data-table-url-state.ts (1)
28-69: Fix stale-closure and query-param clobbering in setUrlState.
- The callback captures a stale urlState because it’s excluded from deps; the ignore comment is incorrect. This can drop concurrent URL changes.
- Building new URLSearchParams() from scratch erases unrelated query params.
Apply both fixes: depend on urlState/searchParams and mutate a cloned params object (delete defaults; set non-defaults).
- // biome-ignore lint/correctness/useExhaustiveDependencies: setSearchParams is stable; urlState is read at call time const setUrlState = useCallback( (newState: Partial<DataTableRouterState>) => { - const updatedState = { ...urlState, ...newState }; - const newParams = new URLSearchParams(); + const updatedState = { ...urlState, ...newState }; + // start from current params to preserve unrelated query keys + const newParams = new URLSearchParams(searchParams); // Only add parameters that are not default values if (updatedState.search !== dataTableRouterParsers.search.defaultValue) { const serialized = dataTableRouterParsers.search.serialize(updatedState.search); - if (serialized !== null) newParams.set('search', serialized); + if (serialized !== null) newParams.set('search', serialized); + } else { + newParams.delete('search'); } if (updatedState.filters.length > 0) { const serialized = dataTableRouterParsers.filters.serialize(updatedState.filters); - if (serialized !== null) newParams.set('filters', serialized); + if (serialized !== null) newParams.set('filters', serialized); + } else { + newParams.delete('filters'); } if (updatedState.page !== dataTableRouterParsers.page.defaultValue) { const serialized = dataTableRouterParsers.page.serialize(updatedState.page); - if (serialized !== null) newParams.set('page', serialized); + if (serialized !== null) newParams.set('page', serialized); + } else { + newParams.delete('page'); } if (updatedState.pageSize !== dataTableRouterParsers.pageSize.defaultValue) { const serialized = dataTableRouterParsers.pageSize.serialize(updatedState.pageSize); - if (serialized !== null) newParams.set('pageSize', serialized); + if (serialized !== null) newParams.set('pageSize', serialized); + } else { + newParams.delete('pageSize'); } if (updatedState.sortField !== dataTableRouterParsers.sortField.defaultValue) { const serialized = dataTableRouterParsers.sortField.serialize(updatedState.sortField); - if (serialized !== null) newParams.set('sortField', serialized); + if (serialized !== null) newParams.set('sortField', serialized); + } else { + newParams.delete('sortField'); } if (updatedState.sortOrder !== dataTableRouterParsers.sortOrder.defaultValue) { const serialized = dataTableRouterParsers.sortOrder.serialize(updatedState.sortOrder); - if (serialized !== null) newParams.set('sortOrder', serialized); + if (serialized !== null) newParams.set('sortOrder', serialized); + } else { + newParams.delete('sortOrder'); } // Update the URL with the new search parameters setSearchParams(newParams, { replace: true }); }, - [setSearchParams], + [urlState, searchParams, setSearchParams], );packages/components/src/remix-hook-form/password-field.tsx (1)
1-33: Forward refs per React 19 patterns — duplicate export foundpackages/components/src/remix-hook-form/password-field.tsx defines PasswordField twice (≈ lines 11–13 and 52–54). Reconcile the duplicates (remove/rename/merge) so there is a single exported component. Then change the type-only import to a value import (
import * as React from 'react';) and wrap the exported component withReact.forwardRef<HTMLInputElement, PasswordFieldProps>, forwardingrefintoBasePasswordField.packages/components/src/remix-hook-form/text-field.tsx (1)
1-28: Convert both TextField components to forwardRef and use a value React import.
- packages/components/src/ui/text-field.tsx — change to export TextField via React.forwardRef<HTMLInputElement, TextInputProps>(function TextField(props, ref) { ... }) (remove
reffrom the props destructure, pass the ref to the InputComponent), and switchimport type * as React→import * as React from 'react'.- packages/components/src/remix-hook-form/text-field.tsx — switch to
import * as React from 'react'and export the wrapper with React.forwardRef<HTMLInputElement, TextFieldProps>(function TextField(props, ref) { const { control } = useRemixFormContext(); ... return <BaseTextField ref={ref} control={control} components={components} {...props} />; }); set displayName.Rationale: the UI TextField currently expects
refas a normal prop (not a forwarded ref), so making only the wrapper forward the ref and passing it as the specialrefattribute will not deliver the ref to the input. Both files must be converted to forwardRef to correctly accept and forward refs.packages/components/src/remix-hook-form/phone-input.tsx (1)
1-31: Adopt forwardRef in both components to fix broken ref forwarding.PhoneInput (packages/components/src/remix-hook-form/phone-input.tsx) currently treats ref as a prop and will not receive JSX refs; PhoneInputField (packages/components/src/ui/phone-input-field.tsx) reads ref from props instead of exposing a forwarded ref. Convert both to React.forwardRef and use a value React import.
- packages/components/src/remix-hook-form/phone-input.tsx — change the import to a value import and wrap the component with React.forwardRef<HTMLInputElement, PhoneInputProps>, accept ref as the second arg, and pass it to BasePhoneInputField (ref={ref}).
- packages/components/src/ui/phone-input-field.tsx — remove ref from the prop type, convert the component to React.forwardRef<HTMLInputElement, PhoneInputFieldProps>, and forward the ref to the underlying InputComponent (ref={ref}). Update components.Input typing to accept forwarded refs (ForwardRefExoticComponent/RefAttributes or an innerRef pattern).
- Verify packages/components/src/ui/phone-input.tsx (PhoneNumberInput) forwards refs; convert it to forwardRef if it does not.
apps/docs/src/remix-hook-form/switch.stories.tsx (1)
54-74: Use fetcher.Form instead of a regular form in docs stories.Guidelines require fetcher.Form for Router-integrated stories.
- <RemixFormProvider {...methods}> - <form onSubmit={methods.handleSubmit}> + <RemixFormProvider {...methods}> + <fetcher.Form onSubmit={methods.handleSubmit}> <div className="space-y-4"> <Switch name="notifications" label="Enable notifications" /> <Switch name="marketing" label="Receive marketing emails" /> <Button type="submit" className="mt-4"> Submit </Button> {fetcher.data && ( <div className="mt-4"> <p className="text-sm font-medium">Submitted preferences:</p> <p className="text-sm text-gray-500"> Notifications: {fetcher.data.notifications ? 'Enabled' : 'Disabled'} </p> <p className="text-sm text-gray-500"> Marketing emails: {fetcher.data.marketing ? 'Enabled' : 'Disabled'} </p> </div> )} </div> - </form> + </fetcher.Form> </RemixFormProvider>apps/docs/src/remix-hook-form/data-table/data-table-server-driven.stories.tsx (1)
98-107: Guard against empty datasets before using the “in” operator.
sortField in mockDatabase[0]throws if the array is empty.- if (sortField && sortField in mockDatabase[0]) { + if (sortField && processedData.length > 0 && sortField in processedData[0]!) { processedData.sort((a, b) => { - const aValue = a[sortField as keyof MockIssue]; - const bValue = b[sortField as keyof MockIssue]; + const aValue = a[sortField as keyof MockIssue]; + const bValue = b[sortField as keyof MockIssue]; let comparison = 0; if (aValue < bValue) comparison = -1; if (aValue > bValue) comparison = 1; return sortOrder === 'desc' ? comparison * -1 : comparison; }); }apps/docs/src/remix-hook-form/form-error-custom.stories.tsx (1)
36-36: Fix mismatch between rendered class and test assertion.Rendered class uses
text-red-800but test expectstext-destructive.- <FormMessage className="text-red-800 font-medium" {...props} /> + <FormMessage className="text-destructive font-medium" {...props} />Alternatively, update the assertion to match the chosen token, but prefer the design token (
text-destructive) for consistency.Also applies to: 230-230
packages/components/src/ui/otp-input-field.tsx (1)
69-79: Forward OTPInput props; current implementation drops them.OTPInputFieldProps extends OTPInput props, but they aren’t spread into .
export function OTPInputField({ control, name, label, description, className, labelClassName, inputClassName, maxLength, components, -}: OTPInputFieldProps) { + ...otpProps +}: OTPInputFieldProps) { @@ <FormControl Component={components?.FormControl}> <OTPInput id={formItemId} aria-describedby={formItemId} value={field.value} onChange={field.onChange} className={inputClassName} maxLength={maxLength} data-slot="otp-input-field" + {...otpProps} >Also applies to: 94-116
apps/docs/src/remix-hook-form/checkbox-custom.stories.tsx (2)
3-3: Type‑only import breakstypeofusage. Import values instead.
import type { FormLabel, FormMessage }makesFormLabel/FormMessageunavailable as values, sotypeof FormLabel/typeof FormMessagein prop types won’t compile.Apply this diff:
-import type { FormLabel, FormMessage } from '@lambdacurry/forms/remix-hook-form/form'; +import { FormLabel, FormMessage } from '@lambdacurry/forms/remix-hook-form/form';Also applies to: 43-50, 54-62
67-74: Remove unsafe literal type assertions on booleans.
false as trueis an unsound cast and can hide bugs. Default values should be plain booleans.Apply this diff:
- terms: false as true, + terms: false, - required: false as true, + required: false,Repeat the same changes in the other two
defaultValuesblocks.Also applies to: 106-114, 147-153
apps/docs/src/remix-hook-form/radio-group.stories.tsx (1)
43-47: Missing imports for React types and Label component.
ComponentType,ComponentPropsWithoutRef, andLabelare used but not imported, causing compile errors.Apply this diff:
+import type { ComponentPropsWithoutRef, ComponentType } from 'react'; +import { Label } from '@lambdacurry/forms/ui/label';apps/docs/src/remix-hook-form/text-field-custom.stories.tsx (2)
133-144: Move decorators from meta to the story to match docs guidelines.Per repo guidelines, decorators must be on individual stories, not in meta.
Apply:
const meta: Meta<typeof TextField> = { title: 'RemixHookForm/TextField Customized', component: TextField, parameters: { layout: 'centered' }, tags: ['autodocs'], - decorators: [ - withReactRouterStubDecorator({ - routes: [ - { - path: '/', - Component: CustomTextFieldExample, - action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), - }, - ], - }), - ], } satisfies Meta<typeof TextField>; export default meta; type Story = StoryObj<typeof meta>; export const CustomComponents: Story = { + decorators: [ + withReactRouterStubDecorator({ + routes: [ + { + path: '/', + Component: CustomTextFieldExample, + action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), + }, + ], + }), + ], render: () => <CustomTextFieldExample />,
258-277: Prefer findBy and cover invalid → valid flow in play().*Stories should include default, invalid, and valid phases; and use findBy* for async elements.
Apply:
- play: async ({ canvasElement }) => { - const canvas = within(canvasElement); - - // Fill in the form fields - const usernameInput = canvas.getByPlaceholderText('Enter your username'); - const emailInput = canvas.getByPlaceholderText('Enter your email'); - const passwordInput = canvas.getByPlaceholderText('Enter your password'); + play: async ({ canvasElement, step }) => { + const canvas = within(canvasElement); + + await step('Invalid submission shows errors', async () => { + const submitButton = await canvas.findByRole('button', { name: 'Submit' }); + await userEvent.click(submitButton); + await expect(canvas.findByText(/at least 3 characters/i)).resolves.toBeInTheDocument(); + await expect(canvas.findByText(/valid email/i)).resolves.toBeInTheDocument(); + await expect(canvas.findByText(/at least 8 characters/i)).resolves.toBeInTheDocument(); + }); + + // Fill in the form fields (async-safe queries) + const usernameInput = await canvas.findByPlaceholderText('Enter your username'); + const emailInput = await canvas.findByPlaceholderText('Enter your email'); + const passwordInput = await canvas.findByPlaceholderText('Enter your password'); @@ - const submitButton = canvas.getByRole('button', { name: 'Submit' }); + const submitButton = await canvas.findByRole('button', { name: 'Submit' }); await userEvent.click(submitButton); @@ - await expect(await canvas.findByText('Form submitted successfully')).toBeInTheDocument(); + await expect(canvas.findByText('Form submitted successfully')).resolves.toBeInTheDocument();apps/docs/src/remix-hook-form/calendar-with-month-year-select.stories.tsx (2)
99-142: Use fetcher.Form instead of Form in stories with router integration.Guideline: always use fetcher.Form for Remix/Router integration in stories.
Apply:
- <RemixFormProvider {...methods}> - <Form onSubmit={methods.handleSubmit}> + <RemixFormProvider {...methods}> + <fetcher.Form onSubmit={methods.handleSubmit}> @@ - </Form> + </fetcher.Form> </RemixFormProvider>Also remove the unused Form import from react-router.
189-236: Add invalid phase and prefer findBy for async elements.*Covers the required three phases and stabilizes queries.
Apply:
- play: async ({ canvasElement, step }) => { + play: async ({ canvasElement, step }) => { const canvas = within(canvasElement); @@ - await step('Verify initial state', async () => { - // Verify the dropdown select button is present with default text - const dropdownSelectButton = await canvas.findByText('Month and Year'); + await step('Verify initial state', async () => { + const dropdownSelectButton = await canvas.findByText('Month and Year'); expect(dropdownSelectButton).toBeInTheDocument(); - // Verify submit button is present but form hasn't been submitted yet - const submitButton = canvas.getByRole('button', { name: /submit/i }); + const submitButton = await canvas.findByRole('button', { name: /submit/i }); expect(submitButton).toBeInTheDocument(); expect(canvas.queryByText('Submitted with:')).not.toBeInTheDocument(); }); + + await step('Invalid submission shows date required', async () => { + const submitButton = await canvas.findByRole('button', { name: /submit/i }); + await userEvent.click(submitButton); + await expect(canvas.findByText(/please select a date/i)).resolves.toBeInTheDocument(); + });packages/components/src/ui/data-table/data-table.tsx (1)
6-12: Export the props type for consumers.Repo guideline: export component and props type from components package.
Apply:
-interface DataTableProps<TData> { +export interface DataTableProps<TData> { table: TableType<TData>; columns: number; onPaginationChange?: (pageIndex: number, pageSize: number) => void; pageCount?: number; className?: string; }apps/docs/src/remix-hook-form/password-field.stories.tsx (1)
4-6: Fix canvas usage in test helpers; current code will fail at runtime.Test helpers destructure
canvasfrom StoryContext, but Storybook providescanvasElement. Create a canvas viawithin(canvasElement)and pass it explicitly.Apply:
-import { expect, userEvent } from '@storybook/test'; +import { expect, userEvent, within } from '@storybook/test'; @@ -type Test = StoryObj<typeof meta>; +type Test = StoryObj<typeof meta>; +type TestCtx = { canvas: ReturnType<typeof within> }; @@ -const testDefaultValues = ({ canvas }: StoryContext) => { +const testDefaultValues = ({ canvas }: TestCtx) => { @@ -const testPasswordVisibilityToggle = async ({ canvas }: StoryContext) => { +const testPasswordVisibilityToggle = async ({ canvas }: TestCtx) => { @@ -const testWeakPasswordValidation = async ({ canvas }: StoryContext) => { +const testWeakPasswordValidation = async ({ canvas }: TestCtx) => { @@ -const testPasswordMismatchValidation = async ({ canvas }: StoryContext) => { +const testPasswordMismatchValidation = async ({ canvas }: TestCtx) => { @@ -const testValidSubmission = async ({ canvas }: StoryContext) => { +const testValidSubmission = async ({ canvas }: TestCtx) => { @@ -const testRefFunctionality = async ({ canvas }: StoryContext) => { +const testRefFunctionality = async ({ canvas }: TestCtx) => { @@ export const CreateAccountExample: Story = { - play: async (storyContext) => { - testDefaultValues(storyContext); - await testPasswordVisibilityToggle(storyContext); - await testWeakPasswordValidation(storyContext); - await testPasswordMismatchValidation(storyContext); - await testValidSubmission(storyContext); - await testRefFunctionality(storyContext); + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + testDefaultValues({ canvas }); + await testPasswordVisibilityToggle({ canvas }); + await testWeakPasswordValidation({ canvas }); + await testPasswordMismatchValidation({ canvas }); + await testValidSubmission({ canvas }); + await testRefFunctionality({ canvas }); },Also applies to: 116-123, 211-219
apps/docs/src/remix-hook-form/form-error-basic.stories.tsx (2)
124-135: Move decorators from meta to the story.Meta-level decorators violate the docs rules; place them on the story instead.
Apply:
const meta: Meta<typeof FormError> = { title: 'RemixHookForm/FormError/Basic', component: FormError, parameters: { @@ }, tags: ['autodocs'], - decorators: [ - withReactRouterStubDecorator({ - routes: [ - { - path: '/', - Component: BasicFormErrorExample, - action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), - }, - ], - }), - ], } satisfies Meta<typeof FormError>; @@ export const Default: Story = { + decorators: [ + withReactRouterStubDecorator({ + routes: [ + { + path: '/', + Component: BasicFormErrorExample, + action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), + }, + ], + }), + ],
157-199: Add a valid submission phase to complete the 3-phase test flow.Include success path after invalid paths.
Apply:
play: async ({ canvasElement, step }) => { const canvas = within(canvasElement); @@ await step('Test form-level error with invalid credentials', async () => { @@ }); + + await step('Test successful login', async () => { + const emailInput = await canvas.findByLabelText(/email address/i); + const passwordInput = await canvas.findByLabelText(/password/i); + await userEvent.clear(emailInput); + await userEvent.clear(passwordInput); + await userEvent.type(emailInput, 'user@example.com'); + await userEvent.type(passwordInput, 'password123'); + const submitButton = await canvas.findByRole('button', { name: /sign in/i }); + await userEvent.click(submitButton); + await expect(canvas.findByText(/login successful/i)).resolves.toBeInTheDocument(); + });apps/docs/src/remix-hook-form/textarea.stories.tsx (1)
80-91: Relocate decorators from meta to story level.Keep meta clean; attach router decorator to the story.
Apply:
const meta: Meta<typeof Textarea> = { title: 'RemixHookForm/Textarea', component: Textarea, parameters: { layout: 'centered' }, tags: ['autodocs'], - decorators: [ - withReactRouterStubDecorator({ - routes: [ - { - path: '/', - Component: ControlledTextareaExample, - action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), - }, - ], - }), - ], } satisfies Meta<typeof Textarea>; @@ export const Default: Story = { + decorators: [ + withReactRouterStubDecorator({ + routes: [ + { + path: '/', + Component: ControlledTextareaExample, + action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), + }, + ], + }), + ],apps/docs/src/remix-hook-form/date-picker.stories.tsx (3)
50-66: Use fetcher.Form instead of Form.Aligns with router integration guideline and consistency with other stories.
Apply:
- <RemixFormProvider {...methods}> - <Form onSubmit={methods.handleSubmit}> + <RemixFormProvider {...methods}> + <fetcher.Form onSubmit={methods.handleSubmit}> @@ - </Form> + </fetcher.Form> </RemixFormProvider>Remove the unused Form import.
100-156: Add invalid phase and prefer findBy queries.*Covers required phases and stabilizes async rendering.
Apply:
- play: async ({ canvasElement, step }) => { + play: async ({ canvasElement, step }) => { const canvas = within(canvasElement); await step('Verify initial state', async () => { - // Verify the date picker trigger is present and has correct label - const datePickerTrigger = canvas.getByRole('button', { name: /select a date/i }); + const datePickerTrigger = await canvas.findByRole('button', { name: /select a date/i }); expect(datePickerTrigger).toBeInTheDocument(); - // Verify submit button is present but form hasn't been submitted yet - const submitButton = canvas.getByRole('button', { name: /submit/i }); + const submitButton = await canvas.findByRole('button', { name: /submit/i }); expect(submitButton).toBeInTheDocument(); expect(canvas.queryByText('Submitted with date:')).not.toBeInTheDocument(); }); + + await step('Invalid submission shows date required', async () => { + const submitButton = await canvas.findByRole('button', { name: /submit/i }); + await userEvent.click(submitButton); + await expect(canvas.findByText(/please select a date/i)).resolves.toBeInTheDocument(); + });
84-94: Move decorators from meta to the story.Meta-level decorators violate the docs rule.
Apply:
const meta: Meta<typeof DatePicker> = { title: 'RemixHookForm/Date Picker', component: DatePicker, parameters: { layout: 'centered' }, tags: ['autodocs'], - decorators: [ - withReactRouterStubDecorator({ - routes: [ - { - path: '/', - Component: ControlledDatePickerExample, - action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), - }, - ], - }), - ], } satisfies Meta<typeof DatePicker>; @@ export const Default: Story = { + decorators: [ + withReactRouterStubDecorator({ + routes: [ + { + path: '/', + Component: ControlledDatePickerExample, + action: async ({ request }: ActionFunctionArgs) => handleFormSubmission(request), + }, + ], + }), + ],apps/docs/src/remix-hook-form/otp-input.stories.tsx (1)
45-61: Use fetcher.Form instead of Form in stories requiring Router integrationGuideline requires fetcher.Form. Replace the plain Form usage.
- return ( - <RemixFormProvider {...methods}> - <Form onSubmit={methods.handleSubmit}> + return ( + <RemixFormProvider {...methods}> + <fetcher.Form onSubmit={methods.handleSubmit}> <div className="space-y-4"> <OTPInput name="otp" label="Enter OTP" maxLength={6} /> <Button type="submit" className="mt-4"> Submit </Button> {fetcher.data?.otp && ( <div className="mt-4"> <p className="text-sm font-medium">Submitted OTP:</p> <p className="text-sm text-gray-500">{fetcher.data.otp}</p> </div> )} </div> - </Form> + </fetcher.Form> </RemixFormProvider>Also drop the unused Form import:
-import { type ActionFunctionArgs, Form, useFetcher } from 'react-router'; +import { type ActionFunctionArgs, useFetcher } from 'react-router';apps/docs/src/remix-hook-form/checkbox-list.stories.tsx (1)
212-217:playpasses the wrong shape to helpers (canvas never created)You pass Storybook’s context directly to helpers expecting
{ canvas }. Create the canvas and pass it.-export const Tests: Story = { +export const Tests: Story = { parameters: { docs: { /* ... */ }, }, - play: async (storyContext) => { - testDefaultValues(storyContext); - await testErrorState(storyContext); - await testColorSelection(storyContext); + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const ctx = { canvas }; + testDefaultValues(ctx); + await testErrorState(ctx); + await testColorSelection(ctx); }, };apps/docs/src/remix-hook-form/data-table/data-table-router-form.stories.tsx (1)
287-297: Move decorators from meta to the story per docs guidelines.Our docs rule: “Always place decorators on individual stories; never in meta.”
Apply:
const meta = { title: 'RemixHookForm/Data Table', component: DataTableRouterForm, parameters: { layout: 'fullscreen', docs: { /* ... */ }, }, - decorators: [ - withReactRouterStubDecorator({ - routes: [{ path: '/', Component: DataTableRouterFormExample, loader: handleDataFetch }], - }), - ], tags: ['autodocs'], } satisfies Meta; export default meta; type Story = StoryObj<typeof meta>; export const Default: Story = { args: {} satisfies Record<string, unknown>, render: () => <DataTableRouterFormExample />, + decorators: [ + withReactRouterStubDecorator({ + routes: [{ path: '/', Component: DataTableRouterFormExample, loader: handleDataFetch }], + }), + ], parameters: { /* ... */ }, };packages/components/src/ui/data-table-filter/components/filter-value.tsx (3)
171-177: Bug: treating a component type as an element breaks icon rendering.You cast icon to ElementType then call isValidElement on it. If icon is a ReactElement, JSX with Icon as a “component” will throw; if it’s a component, isValidElement returns false. Use the original value for the check and clone/add key when it is an element.
Apply:
- {hasOptionIcons && - take(selected, 3).map(({ value, icon }) => { - const Icon = icon as ElementType; - return isValidElement(Icon) ? Icon : <Icon key={value} className="size-4" />; - })} + {hasOptionIcons && + take(selected, 3).map(({ value, icon }) => { + if (isValidElement(icon)) return cloneElement(icon, { key: value }); + const IconComp = icon as ElementType; + return <IconComp key={value} className="size-4" />; + })}
210-214: Bug: same element-vs-component issue in multi option path.Clone when element; otherwise render the component type.
- {take(selected, 3).map(({ value, icon }) => { - const Icon = icon as ElementType; - return isValidElement(Icon) ? cloneElement(Icon, { key: value }) : <Icon key={value} className="size-4" />; - })} + {take(selected, 3).map(({ value, icon }) => { + if (isValidElement(icon)) return cloneElement(icon, { key: value }); + const IconComp = icon as ElementType; + return <IconComp key={value} className="size-4" />; + })}
647-651: Bug: inverted equality check prevents state sync.The effect only updates when arrays are equal; it should update when they differ.
-useEffect(() => { - if (filter?.values && filter.values.length === values.length && filter.values.every((v, i) => v === values[i])) { - setValues(filter.values); - } -}, [filter?.values, values]); +useEffect(() => { + if ( + filter?.values && + (filter.values.length !== values.length || !filter.values.every((v, i) => v === values[i])) + ) { + setValues(filter.values); + } +}, [filter?.values, values]);packages/components/src/remix-hook-form/data-table-router-form.tsx (3)
100-105: Sorting state should be empty when no column is selectedPassing
{ id: '' }yields an invalid sort state. Fall back to[].- sorting: [{ id: urlState.sortField, desc: urlState.sortOrder === 'desc' }], + sorting: urlState.sortField + ? [{ id: urlState.sortField, desc: urlState.sortOrder === 'desc' }] + : [], @@ - const sorting = typeof updater === 'function' ? updater(currentSorting) : updater; + const sorting = typeof updater === 'function' ? updater(currentSorting) : updater; setUrlState({ - sortField: sorting[0]?.id ?? '', - sortOrder: sorting[0]?.desc ? 'desc' : 'asc', + sortField: sorting[0]?.id ?? '', + sortOrder: sorting[0]?.desc ? 'desc' : 'asc', page: 0, });Also applies to: 120-128
135-137: SSR risk: directwindow.cryptousage
windowis undefined during SSR (Remix). UseglobalThis.crypto?.randomUUIDwith a safe fallback.- const skeletonRowIds = useMemo(() => { - const count = urlState.pageSize > 0 ? urlState.pageSize : defaultDataTableState.pageSize; - return Array.from({ length: count }, () => window.crypto.randomUUID()); - }, [urlState.pageSize, defaultDataTableState.pageSize]); + const genId = () => globalThis.crypto?.randomUUID?.() ?? Math.random().toString(36).slice(2); + const skeletonRowIds = useMemo(() => { + const count = urlState.pageSize > 0 ? urlState.pageSize : defaultDataTableState.pageSize; + return Array.from({ length: count }, genId); + }, [urlState.pageSize, defaultDataTableState.pageSize]);
224-226: IncorrectcolSpanwhen columns are hiddenUsing
columns.lengthbreaks layout if some columns are hidden. UsevisibleColumns.length.- <TableCell colSpan={columns.length} className="h-24 text-center"> + <TableCell colSpan={visibleColumns.length} className="h-24 text-center">packages/components/src/ui/data-table-filter/lib/array.ts (1)
55-88: CorrectdeepEqualfor Dates/RegExps and tighten object checksTwo different Dates currently compare equal (no own keys). Add explicit handling.
function deepEqual(a: unknown, b: unknown): boolean { // Check strict equality first. if (a === b) return true; // If types differ, they’re not equal. if (typeof a !== typeof b) return false; if (a === null || b === null || a === undefined || b === undefined) return false; + // Dates + if (a instanceof Date || b instanceof Date) { + return a instanceof Date && b instanceof Date && a.getTime() === b.getTime(); + } + // RegExps + if (a instanceof RegExp || b instanceof RegExp) { + return a instanceof RegExp && b instanceof RegExp && a.source === b.source && a.flags === b.flags; + } + // Check arrays. if (Array.isArray(a)) { if (!Array.isArray(b) || a.length !== b.length) return false; for (let i = 0; i < a.length; i++) { if (!deepEqual(a[i], (b as unknown[])[i])) return false; } return true; } // Check objects. if (typeof a === 'object') { if (typeof b !== 'object') return false; const aObj = a as Record<string, unknown>; const bObj = b as Record<string, unknown>; const aKeys = Object.keys(aObj).sort(); const bKeys = Object.keys(bObj).sort(); if (aKeys.length !== bKeys.length) return false; for (let i = 0; i < aKeys.length; i++) { if (aKeys[i] !== bKeys[i]) return false; if (!deepEqual(aObj[aKeys[i]], bObj[bKeys[i]])) return false; } return true; } // For any other types (should be primitives by now), use strict equality. return false; }packages/components/src/ui/select.tsx (1)
126-147: Combobox ARIA applied to Trigger instead of the text inputFor a searchable select, the input should be the combobox owner. Move ARIA to
SearchInputand drop it from the button.- <Trigger + <Trigger ref={triggerRef} disabled={disabled} @@ - role="combobox" - aria-haspopup="listbox" - aria-expanded={popoverState.isOpen} - aria-controls={listboxId} + aria-haspopup="listbox"And on
SearchInput:- <SearchInput + <SearchInput type="text" value={query} @@ - aria-activedescendant={filtered.length > 0 ? `${listboxId}-option-${activeIndex}` : undefined} + role="combobox" + aria-expanded={popoverState.isOpen} + aria-controls={listboxId} + aria-autocomplete="list" + aria-activedescendant={filtered.length > 0 ? `${listboxId}-option-${activeIndex}` : undefined}packages/components/src/ui/data-table-filter/core/filters.ts (1)
236-243: Type bug: transform branch returns ColumnOption[] but is typed as element value array.
column.transformOptionFnreturnsColumnOption, yet the code casts the result toElementType<NonNullable<TVal>>, weakening type guarantees for downstream calls.Apply:
-export function getColumnValues<TData, TType extends ColumnDataType, TVal>( - column: ColumnConfig<TData, TType, TVal>, - data: TData[], -) { +export function getColumnValues<TData, TType extends ColumnDataType, TVal>( + column: ColumnConfig<TData, TType, TVal>, + data: TData[], +): ElementType<NonNullable<TVal>>[] | ColumnOption[] { @@ - if (column.transformOptionFn) { + if (column.transformOptionFn) { const memoizedTransform = memo( () => [raw], - (deps) => deps[0].map((v) => column.transformOptionFn?.(v) as ElementType<NonNullable<TVal>>), + (deps) => deps[0].map((v) => column.transformOptionFn!(v)), { key: `transform-values-${column.id}` }, ); - return memoizedTransform(); + return memoizedTransform(); }And in
createColumnsupdate the local type:-const getValues: () => ElementType<NonNullable<unknown>>[] = memo( +const getValues: () => (ElementType<NonNullable<unknown>>[] | ColumnOption[]) = memo(
| import { FormMessage } from '@lambdacurry/forms/remix-hook-form/form'; | ||
| import { Button } from '@lambdacurry/forms/ui/button'; | ||
| import type { Meta, StoryObj } from '@storybook/react-vite'; | ||
| import { expect, userEvent, type within } from '@storybook/test'; |
There was a problem hiding this comment.
Invalid type within import breaks types
within is a value export; importing it as a type is invalid. Either import the value or use a typeof import in the type.
-import { expect, userEvent, type within } from '@storybook/test';
+import { expect, userEvent, within } from '@storybook/test';And keep the type use as ReturnType<typeof within> (now valid). If you prefer to avoid a value import purely for types:
type StoryContext = {
canvas: ReturnType<typeof import('@storybook/test').within>;
};🤖 Prompt for AI Agents
In apps/docs/src/remix-hook-form/checkbox-list.stories.tsx around line 6, the
import incorrectly imports "within" as a type which is a runtime value; change
the import to import { expect, userEvent, within } from '@storybook/test'
(remove the "type" qualifier) and update any type usages to use
ReturnType<typeof within> (or, if you must avoid a value import, switch the type
to: canvas: ReturnType<typeof import('@storybook/test').within>).
| import { type ActionFunctionArgs, Form, useFetcher } from 'react-router'; | ||
| import { RemixFormProvider, getValidatedFormData, useRemixForm } from 'remix-hook-form'; | ||
| import { getValidatedFormData, RemixFormProvider, useRemixForm } from 'remix-hook-form'; | ||
| import { z } from 'zod'; |
There was a problem hiding this comment.
Use fetcher.Form instead of Form to wire responses to useFetcher.
You render <Form> while reading fetcher.data. This won’t populate fetcher.data. Use <fetcher.Form> and drop the Form import per docs guidelines.
-import { type ActionFunctionArgs, Form, useFetcher } from 'react-router';
+import { type ActionFunctionArgs, useFetcher } from 'react-router';
@@
- <Form onSubmit={methods.handleSubmit}>
+ <fetcher.Form onSubmit={methods.handleSubmit}>
@@
- </Form>
+ </fetcher.Form>Also applies to: 64-66
🤖 Prompt for AI Agents
In apps/docs/src/remix-hook-form/radio-group.stories.tsx around lines 7-9 and
also apply to lines 64-66, you are importing and rendering the plain <Form>
while reading fetcher.data which prevents fetcher from populating response data;
remove the Form import from the top, keep useFetcher, and replace any <Form>
usage with <fetcher.Form> in the JSX so responses are wired into fetcher.data
(ensure the component uses the fetcher variable from useFetcher and adjust
imports accordingly).
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "^1.9.4", | ||
| "@biomejs/biome": "2.2.4", |
There was a problem hiding this comment.
🧩 Analysis chain
Pinning @biomejs/biome to 2.2.4 is fine. Ensure biome.json schema is v2.2.4.
Prevents schema/diagnostic drift after upgrade.
Verification snippet:
🏁 Script executed:
#!/bin/bash
rg -nP '"\\$schema"\\s*:\\s*".*biomejs.*2\\.2\\.4' biome.json -S || { echo "Update biome.json $schema to 2.2.4"; exit 1; }Length of output: 27
Pin @biomejs/biome@2.2.4 — update biome.json "$schema" to v2.2.4.
biome.json does not reference the 2.2.4 schema; set the "$schema" property in biome.json to the Biome v2.2.4 schema to avoid schema/diagnostic drift.
File: biome.json
🤖 Prompt for AI Agents
In package.json around line 23, update the project's biome.json to reference the
Biome v2.2.4 JSON schema: open biome.json and set the top-level "$schema"
property to the Biome 2.2.4 schema URL (e.g. the v2.2.4 schema from schemastore)
so the file explicitly pins to the 2.2.4 schema and prevents schema/diagnostic
drift.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
On it! I'll handle adding Biome 2.2.4... 🚀
💻 View my work • 🛑 Stop
| @@ -1,2 +1,2 @@ | |||
| export { useDataTableFilters } from './hooks/use-data-table-filters'; | |||
| export { DataTableFilter } from './components/data-table-filter'; | |||
| export { useDataTableFilters } from './hooks/use-data-table-filters'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Re-export the props type to match our UI export guideline.
Per guideline “Always export both the component and its props type,” also re-export the props type if it exists.
Proposed change:
export { DataTableFilter } from './components/data-table-filter';
export { useDataTableFilters } from './hooks/use-data-table-filters';
+export type { DataTableFilterProps } from './components/data-table-filter';If the props type has a different name, update accordingly. Run the script below to confirm the name:
🏁 Script executed:
#!/bin/bash
fd data-table-filter -t f packages/components/src/ui | xargs rg -nP 'export\s+type\s+\{\s*([A-Za-z0-9_]+)\s*\}\s+from\s+.\./components/data-table-filter'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show re-export file mentioned in review ==="
if [ -f packages/components/src/ui/data-table-filter/index.tsx ]; then
sed -n '1,160p' packages/components/src/ui/data-table-filter/index.tsx || true
else
echo "MISSING: packages/components/src/ui/data-table-filter/index.tsx"
fi
echo
echo "=== list files with 'data-table-filter' in the path under packages/components ==="
fd -HI -t f 'data-table-filter' packages/components || true
echo
echo "=== search repo for common prop-type names and re-export patterns ==="
rg -n --hidden -S 'DataTableFilterProps|export\s+(?:type|interface)\s+[A-Za-z0-9_]*DataTableFilter[A-Za-z0-9_]*|export\s+type\s+\{[^}]+\}\s+from\s+[\x22\x27][^\\x22\\x27]*data-table-filter' || true
echo
echo "=== search for generic exported '*Props' near data-table-filter files ==="
for f in $(fd -HI -t f 'data-table-filter' packages/components || true); do
echo "---- $f ----"
rg -n -C2 --hidden -S 'export\s+(?:type|interface)\s+[A-Za-z0-9_]*Props\b|interface\s+[A-Za-z0-9_]*Props\b|type\s+[A-Za-z0-9_]*Props\b' "$f" || true
done
echo
echo "=== fallback greps across packages if fd/rg missed anything ==="
find packages -type f -iname '*data-table-filter*' -print -maxdepth 6 || true
grep -RIn --line-number --exclude-dir=node_modules -E 'DataTableFilterProps|export\s+(type|interface)\s+[A-Za-z0-9_]*Props' packages || trueLength of output: 8304
Export DataTableFilterProps from component and re-export in index
DataTableFilterProps is declared but not exported; export it in the component file and re-export from the package index.
--- packages/components/src/ui/data-table-filter/components/data-table-filter.tsx
-interface DataTableFilterProps<TData> {
+export interface DataTableFilterProps<TData> { export { DataTableFilter } from './components/data-table-filter';
export { useDataTableFilters } from './hooks/use-data-table-filters';
+export type { DataTableFilterProps } from './components/data-table-filter';📝 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 { DataTableFilter } from './components/data-table-filter'; | |
| export { useDataTableFilters } from './hooks/use-data-table-filters'; | |
| export { DataTableFilter } from './components/data-table-filter'; | |
| export { useDataTableFilters } from './hooks/use-data-table-filters'; | |
| export type { DataTableFilterProps } from './components/data-table-filter'; |
🤖 Prompt for AI Agents
In packages/components/src/ui/data-table-filter/index.tsx around lines 1 to 2,
the type DataTableFilterProps is declared in the component but not exported or
re-exported from the package index; update the component file
(./components/data-table-filter) to export the DataTableFilterProps type (export
type DataTableFilterProps = ... or add export to its declaration) and then
re-export that type from this index file (add an export statement exporting
DataTableFilterProps) so consumers can import the props type from the package
entry point.
| function deepHash(value: unknown, cache = new WeakMap<object, string>()): string { | ||
| // Handle primitives and null/undefined. | ||
| if (value === null) return 'null'; | ||
| if (value === undefined) return 'undefined'; | ||
| const type = typeof value; | ||
| if (type === 'number' || type === 'boolean' || type === 'string') { | ||
| return `${type}:${value.toString()}`; | ||
| return `${type}:${String(value)}`; | ||
| } | ||
| if (type === 'function') { | ||
| // Note: using toString for functions. | ||
| return `function:${value.toString()}`; | ||
| return `function:${String(value)}`; | ||
| } |
There was a problem hiding this comment.
Fix cycles and special objects in deepHash
Current implementation recurses infinitely on cyclic objects and collapses all Dates/RegExps to the same hash, corrupting uniq.
function deepHash(value: unknown, cache = new WeakMap<object, string>()): string {
@@
- if (type === 'function') {
- // Note: using toString for functions.
- return `function:${String(value)}`;
- }
+ if (type === 'function') return `function:${String(value)}`;
+ if (value instanceof Date) return `date:${value.toISOString()}`;
+ if (value instanceof RegExp) return `regexp:${value.source}/${value.flags}`;
@@
if (type === 'object') {
- const obj = value as object;
+ const obj = value as object;
// If we’ve seen this object before, return the cached hash.
- const cached = cache.get(obj);
- if (cached) {
- return cached;
- }
+ const cached = cache.get(obj);
+ if (cached) return cached;
+ // Pre-set to break cycles
+ cache.set(obj, '<<cycle>>');
let hash: string;
if (Array.isArray(value)) {
// Compute hash for each element in order.
hash = `array:[${value.map((v) => deepHash(v, cache)).join(',')}]`;
} else {
// For objects, sort keys to ensure the representation is stable.
- const rec = value as Record<string, unknown>;
+ const rec = value as Record<string, unknown>;
const keys = Object.keys(rec).sort();
const props = keys.map((k) => `${k}:${deepHash(rec[k], cache)}`).join(',');
hash = `object:{${props}}`;
}
cache.set(obj, hash);
return hash;
}
// Fallback if no case matched.
return `${type}:${String(value)}`;
}Also applies to: 24-45, 47-49
| if (!prevDeps || cachedResult === undefined || !shallowEqual(prevDeps, deps)) { | ||
| cachedResult = compute(deps); | ||
| prevDeps = deps; | ||
| } else { | ||
| } | ||
|
|
||
| return cachedResult!; | ||
| return cachedResult as TResult; | ||
| }; |
There was a problem hiding this comment.
Recompute-on-undefined causes perpetual recomputation
Using cachedResult === undefined as a cache-miss flag breaks when compute legitimately returns undefined; it will recompute every call. Track “has value” separately.
Apply this diff:
let prevDeps: TDeps | undefined;
- let cachedResult: TResult | undefined;
+ let cachedResult: TResult | undefined;
+ let hasCachedResult = false;
@@
- // If no previous deps or deps have changed, recompute
- if (!prevDeps || cachedResult === undefined || !shallowEqual(prevDeps, deps)) {
+ // Recompute on first run or when deps change
+ if (!prevDeps || !hasCachedResult || !shallowEqual(prevDeps, deps)) {
cachedResult = compute(deps);
prevDeps = deps;
+ hasCachedResult = true;
}
- return cachedResult as TResult;
+ return cachedResult as TResult;📝 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.
| if (!prevDeps || cachedResult === undefined || !shallowEqual(prevDeps, deps)) { | |
| cachedResult = compute(deps); | |
| prevDeps = deps; | |
| } else { | |
| } | |
| return cachedResult!; | |
| return cachedResult as TResult; | |
| }; | |
| let prevDeps: TDeps | undefined; | |
| let cachedResult: TResult | undefined; | |
| let hasCachedResult = false; | |
| return (deps: TDeps) => { | |
| // Recompute on first run or when deps change | |
| if (!prevDeps || !hasCachedResult || !shallowEqual(prevDeps, deps)) { | |
| cachedResult = compute(deps); | |
| prevDeps = deps; | |
| hasCachedResult = true; | |
| } | |
| return cachedResult as TResult; | |
| }; |
🤖 Prompt for AI Agents
In packages/components/src/ui/data-table-filter/lib/memo.ts around lines 13 to
19, the cache-miss check uses cachedResult === undefined which forces
recomputation when compute legitimately returns undefined; change the memo to
track cache presence with a separate boolean (e.g., hasCachedResult = false),
replace the cachedResult === undefined check with !hasCachedResult, set
hasCachedResult = true immediately after assigning cachedResult = compute(deps),
and keep the existing prevDeps shallowEqual logic so that undefined is treated
as a valid cached value.
| export * from '../../remix-hook-form/data-table-router-form'; | ||
| export * from '../../remix-hook-form/data-table-router-toolbar'; |
There was a problem hiding this comment.
Layering violation: UI index should not re-export Remix Hook Form modules
Guideline: “Don't import from remix-hook-form package in ui components.” Move these two re-exports to a higher-level barrel (e.g., packages/components/src/remix-hook-form/index.ts) or the top-level package index.
-export * from '../../remix-hook-form/data-table-router-form';
-export * from '../../remix-hook-form/data-table-router-toolbar';
+// Re-exported from remix-hook-form barrel; keep ui/ isolatedAdditional snippet to add in packages/components/src/remix-hook-form/index.ts:
export * from './data-table-router-form';
export * from './data-table-router-toolbar';🤖 Prompt for AI Agents
In packages/components/src/ui/data-table/index.ts around lines 1 to 2, the file
is re-exporting modules from the remix-hook-form layer which violates layering
rules; remove those two re-export lines from this UI index, create or update
packages/components/src/remix-hook-form/index.ts to re-export
'./data-table-router-form' and './data-table-router-toolbar', and update any
imports elsewhere to import from packages/components/src/remix-hook-form (or the
package top-level barrel) instead of the ui/data-table index.
| @@ -1,5 +1,5 @@ | |||
| import { Popover } from '@radix-ui/react-popover'; | |||
| import * as PopoverPrimitive from '@radix-ui/react-popover'; | |||
| import { Popover } from '@radix-ui/react-popover'; | |||
There was a problem hiding this comment.
Incorrect Popover import; will not compile
@radix-ui/react-popover does not export Popover. Use your local wrapper or primitives consistently.
-import { Popover } from '@radix-ui/react-popover';
+import { Popover } from './popover';
// or switch entirely to primitives:
// <PopoverPrimitive.Root open=... onOpenChange=...>📝 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.
| import { Popover } from '@radix-ui/react-popover'; | |
| import { Popover } from './popover'; |
🤖 Prompt for AI Agents
In packages/components/src/ui/select.tsx around line 2, the import "import {
Popover } from '@radix-ui/react-popover';" is incorrect and will not compile;
replace it with the project’s local Popover wrapper/primitives import (e.g., the
relative path to your UI popover module) and ensure you use the exact export
shape (named vs default) the local module provides so the component compiles and
matches the rest of your primitives.
| {filtered.length === 0 && <li className="px-3 py-2 text-sm text-gray-500">No results.</li>} | ||
| {filtered.map((option, index) => { | ||
| const isSelected = option.value === value; | ||
| const isActive = index === activeIndex; | ||
| return ( | ||
| <li key={option.value} className="list-none"> | ||
| <Item | ||
| ref={isSelected ? selectedItemRef : undefined} | ||
| onClick={() => { | ||
| onValueChange?.(option.value); | ||
| setQuery(''); | ||
| popoverState.close(); | ||
| }} | ||
| className={cn( | ||
| 'w-full text-left cursor-pointer select-none py-3 px-3 transition-colors duration-150 flex items-center gap-2 rounded', | ||
| 'text-gray-900', | ||
| isSelected ? 'bg-gray-100' : 'hover:bg-gray-100', | ||
| isActive && !isSelected && 'bg-gray-50', | ||
| itemClassName, | ||
| )} | ||
| role="option" | ||
| aria-selected={isSelected} | ||
| id={`${listboxId}-option-${index}`} | ||
| data-selected={isSelected ? 'true' : 'false'} | ||
| data-active={isActive ? 'true' : 'false'} | ||
| data-index={index} | ||
| data-value={option.value} | ||
| data-testid={`select-option-${option.value}`} | ||
| selected={isSelected} | ||
| > | ||
| {isSelected && <CheckIcon className="h-4 w-4 flex-shrink-0" />} | ||
| <span className={cn('block truncate', !isSelected && 'ml-6', isSelected && 'font-semibold')}> | ||
| {option.label} | ||
| </span> | ||
| </Item> | ||
| </li> | ||
| ); | ||
| })} | ||
| </ul> |
There was a problem hiding this comment.
Make options unfocusable to avoid stray tab-stops; restore focus after click
With aria-activedescendant, only the input should receive focus. Buttons are tabbable by default.
- <Item
+ <Item
ref={isSelected ? selectedItemRef : undefined}
onClick={() => {
onValueChange?.(option.value);
setQuery('');
popoverState.close();
+ triggerRef.current?.focus();
}}
@@
- role="option"
+ role="option"
+ tabIndex={-1}📝 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.
| {filtered.length === 0 && <li className="px-3 py-2 text-sm text-gray-500">No results.</li>} | |
| {filtered.map((option, index) => { | |
| const isSelected = option.value === value; | |
| const isActive = index === activeIndex; | |
| return ( | |
| <li key={option.value} className="list-none"> | |
| <Item | |
| ref={isSelected ? selectedItemRef : undefined} | |
| onClick={() => { | |
| onValueChange?.(option.value); | |
| setQuery(''); | |
| popoverState.close(); | |
| }} | |
| className={cn( | |
| 'w-full text-left cursor-pointer select-none py-3 px-3 transition-colors duration-150 flex items-center gap-2 rounded', | |
| 'text-gray-900', | |
| isSelected ? 'bg-gray-100' : 'hover:bg-gray-100', | |
| isActive && !isSelected && 'bg-gray-50', | |
| itemClassName, | |
| )} | |
| role="option" | |
| aria-selected={isSelected} | |
| id={`${listboxId}-option-${index}`} | |
| data-selected={isSelected ? 'true' : 'false'} | |
| data-active={isActive ? 'true' : 'false'} | |
| data-index={index} | |
| data-value={option.value} | |
| data-testid={`select-option-${option.value}`} | |
| selected={isSelected} | |
| > | |
| {isSelected && <CheckIcon className="h-4 w-4 flex-shrink-0" />} | |
| <span className={cn('block truncate', !isSelected && 'ml-6', isSelected && 'font-semibold')}> | |
| {option.label} | |
| </span> | |
| </Item> | |
| </li> | |
| ); | |
| })} | |
| </ul> | |
| {filtered.length === 0 && <li className="px-3 py-2 text-sm text-gray-500">No results.</li>} | |
| {filtered.map((option, index) => { | |
| const isSelected = option.value === value; | |
| const isActive = index === activeIndex; | |
| return ( | |
| <li key={option.value} className="list-none"> | |
| <Item | |
| ref={isSelected ? selectedItemRef : undefined} | |
| onClick={() => { | |
| onValueChange?.(option.value); | |
| setQuery(''); | |
| popoverState.close(); | |
| triggerRef.current?.focus(); | |
| }} | |
| className={cn( | |
| 'w-full text-left cursor-pointer select-none py-3 px-3 transition-colors duration-150 flex items-center gap-2 rounded', | |
| 'text-gray-900', | |
| isSelected ? 'bg-gray-100' : 'hover:bg-gray-100', | |
| isActive && !isSelected && 'bg-gray-50', | |
| itemClassName, | |
| )} | |
| role="option" | |
| tabIndex={-1} | |
| aria-selected={isSelected} | |
| id={`${listboxId}-option-${index}`} | |
| data-selected={isSelected ? 'true' : 'false'} | |
| data-active={isActive ? 'true' : 'false'} | |
| data-index={index} | |
| data-value={option.value} | |
| data-testid={`select-option-${option.value}`} | |
| selected={isSelected} | |
| > | |
| {isSelected && <CheckIcon className="h-4 w-4 flex-shrink-0" />} | |
| <span className={cn('block truncate', !isSelected && 'ml-6', isSelected && 'font-semibold')}> | |
| {option.label} | |
| </span> | |
| </Item> | |
| </li> | |
| ); | |
| })} | |
| </ul> |
🤖 Prompt for AI Agents
In packages/components/src/ui/select.tsx around lines 206 to 244, the rendered
option items are focusable/tabbable which creates stray tab stops when using
aria-activedescendant; make each option unfocusable by setting tabIndex={-1} (or
otherwise ensuring the underlying Item element is not keyboard tabbable) and
keep role="option" for screen readers, and after an option is clicked restore
focus to the main input by calling inputRef.current?.focus() (or the appropriate
input ref) inside the onClick handler after popoverState.close(); this removes
tab stops on options while preserving keyboard/ARIA behavior and returns focus
to the input after selection.
- Added a new section for AI Agent Workflows, detailing the `:researcher` role for investigating bugs and implementation questions. - Removed redundant coordination notes from versioning-with-npm.mdc to streamline the document.
jaruesink
temporarily deployed
to
pr-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
AGENTS.md (3)
15-15: Clarify script names; avoid ambiguous| :fixnotation.Readers may read "
yarn lint|:fix" as a literal pipe. Spell out both commands.- - `yarn lint` | `:fix`: Check/auto-fix with Biome. + - `yarn lint`: Check with Biome. + - `yarn lint:fix`: Auto‑fix with Biome.
37-37: Tighten “Required checks” to mirror the new scripts.Explicitly list commands; optionally encourage running the fix variant locally.
- - Required checks: `yarn lint` passes; build succeeds; tests updated/added. + - Required checks: `yarn lint` passes; `yarn build` succeeds; tests updated/added. + - Recommendation: run `yarn lint:fix` locally before pushing.
53-55: Define guardrails for the :researcher agent (scope, approvals, and data policy).Great addition. Add minimal, concrete constraints so usage is safe and auditable.
## AI Agent Workflows - `:researcher` - Investigate bugs/implementation questions and produce evidence-backed recommendations aligned with the constitution and tech stack. + +Guidelines for `:researcher` +- Scope: read‑only repository analysis and web research; no code execution against local/CI/prod services. +- Approvals: may comment on issues/PRs; creating branches/commits requires a maintainer’s explicit request. +- Data policy: do not include secrets, tokens, or PII in prompts, notes, or outputs; redact if encountered. +- Sources: include links/citations for all non‑obvious claims; prefer primary docs/specs. +- Output: summarize findings with assumptions/open questions and proposed next steps. +- Audit: tag comments with `[research]` and include timestamp + tool versions for traceability. +- Reference: replace “constitution” with a concrete doc link (e.g., `SECURITY.md` or `docs/engineering-principles.md`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.agentos/agents/:researcher.md(1 hunks).cursor/rules/versioning-with-npm.mdc(0 hunks)AGENTS.md(3 hunks)
💤 Files with no reviewable changes (1)
- .cursor/rules/versioning-with-npm.mdc
✅ Files skipped from review due to trivial changes (1)
- .agentos/agents/:researcher.md
🔇 Additional comments (1)
AGENTS.md (1)
15-15: Verify Biome v2 commands & docs — partial verificationFound @biomejs/biome@2.2.4; lint scripts use
biome check .andbiome check . --writein: package.json, apps/docs/package.json, packages/components/package.json.rg search for legacy flags (
--apply,format-and-lint,format:fix,eslint,prettier) returned "No files were searched" — absence of legacy commands/docs cannot be confirmed; re-run with corrected globs (example: rg -nS '--apply|format-and-lint(:fix)?' --hidden --glob '!/node_modules/' .) to fully verify.
🚀 Biome v2.2.4 Migration
This PR upgrades from Biome 1.9.4 to 2.2.4, addressing breaking changes while maintaining code quality standards.
✅ Key Changes
🔧 Configuration Highlights
biome migrate --write✅ Breaking Changes Addressed
📊 Migration Results
This clean migration ensures the codebase stays current with the latest Biome tooling while preserving all existing customizations.
📋 Next Steps (after merge)
yarn format-and-lint:fixto apply any additional formatting🤖 Generated with Claude Code
💻 View my work • 👤 Initiated by @jaruesink • About Codegen
⛔ Remove Codegen from PR • 🚫 Ban action checks
Summary by CodeRabbit
New Features
Accessibility
Refactor
Documentation
Chores