WIP: chore(dependencies): update Radix UI components and integrate Bazza UI filters [LC-219] #64
Conversation
…I filters - Updated Radix UI component versions in package.json and yarn.lock for improved functionality and performance. - Refactored DataTableRouterForm to utilize Bazza UI filter configurations, enhancing filtering capabilities. - Adjusted DataTableRouterToolbar to support new Bazza UI filter integration and improved search functionality. - Updated data-table-router-parsers to define and parse Bazza filter items. - Enhanced DataTableRouterFormExample story to demonstrate new filtering features.
|
|
WalkthroughThis update introduces a comprehensive, type-safe, and localized data table filtering system centered around Bazza UI filters. It includes new filter UI components, core logic for filter configuration, operators, and state management hooks. The integration is applied to example stories, with server-driven and client-driven filtering, and extensive utility, localization, and React component enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataTable
participant DataTableFilter
participant useDataTableFilters
participant Loader (Server)
participant URL
User->>DataTable: Interacts with filters/search/pagination
DataTable->>DataTableFilter: Passes current filter state, columns, actions
DataTableFilter->>useDataTableFilters: Manages filter state/actions
DataTable->>URL: Updates URL with filter/search/pagination state
URL->>Loader (Server): Triggers data fetch with current state
Loader (Server)->>DataTable: Returns filtered/sorted/paginated data + faceted counts
DataTable->>DataTableFilter: Updates with new data and faceted info
DataTableFilter->>User: Renders updated filters and table
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 21
🧹 Nitpick comments (28)
packages/components/src/ui/slider.tsx (1)
8-19: Well-structured component with good defaults.The Slider component is correctly typed using React.ComponentProps from the Radix primitive. The useMemo implementation for calculating values is efficient, though the logic is somewhat complex in a single line.
Consider improving readability of the values calculation:
- const _values = React.useMemo( - () => (Array.isArray(value) ? value : Array.isArray(defaultValue) ? defaultValue : [min, max]), - [value, defaultValue, min, max], - ); + const _values = React.useMemo(() => { + if (Array.isArray(value)) return value; + if (Array.isArray(defaultValue)) return defaultValue; + return [min, max]; + }, [value, defaultValue, min, max]);packages/components/src/ui/data-table-filter/lib/i18n.ts (1)
11-13: Consider logging missing translation keys
Currently, missing keys silently fall back to the key itself. You may want to add a console warning or tracking mechanism in the fallback to surface untranslated strings during development.packages/components/src/ui/data-table-filter/hooks/use-unmount.tsx (2)
3-7: Rename parameter for clarity
RenamefunctocallbackoronUnmountto more clearly convey its purpose as the unmount callback.
8-13: Document intentional empty dependency array
Add a comment explaining that the empty deps array ensures the cleanup only runs on unmount, silencing ESLint warnings and clarifying intent:// Run cleanup only on unmount; `funcRef` always points to the latest callback useEffect(() => () => { funcRef.current() }, []);packages/components/src/ui/data-table-filter/components/filter-actions.tsx (1)
1-26: Well-implemented FilterActions component with appropriate memoizationThe
FilterActionscomponent is well-structured and serves a clear purpose - providing a button to clear all active filters. The implementation is clean and follows good practices.Notable aspects:
- Proper use of
memoto optimize rendering performance- Good conditional visibility based on
hasFiltersstate- Responsive design with the text label hidden on mobile screens
- Appropriate use of the destructive button variant for the clear action
- Good implementation of localization support with fallback to English
One minor suggestion:
Consider adding a confirmation dialog or tooltip when clearing all filters, especially if users might have set up complex filter combinations that would be lost with a single click.
packages/components/src/ui/data-table-filter/lib/memo.ts (2)
1-8: Consider stricter typing for the memoization utilityThe memoization utility uses
anyin the type definition which reduces type safety. Consider using more specific types or generic constraints.-export function memo<TDeps extends readonly any[], TResult>( +export function memo<TDeps extends readonly unknown[], TResult>( getDeps: () => TDeps, compute: (deps: TDeps) => TResult, options: { key: string }, ): () => TResult { let prevDeps: TDeps | undefined let cachedResult: TResult | undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
10-21: Remove commented-out console.logsThere are several commented-out console.log statements. These should be removed or replaced with a proper logging mechanism if debugging is needed.
return () => { - // console.log(`[memo] Calling memoized function: ${options.key}`) const deps = getDeps() // If no previous deps or deps have changed, recompute if (!prevDeps || !shallowEqual(prevDeps, deps)) { - // console.log(`[memo] Cache MISS - ${options.key}`) cachedResult = compute(deps) prevDeps = deps } else { - // console.log(`[memo] Cache HIT - ${options.key}`) }packages/components/src/ui/debounced-input.tsx (2)
22-29: Consider explaining why exhaustive dependencies are ignoredThe comment
biome-ignore lint/correctness/useExhaustiveDependencies: from Bazza UIdoesn't provide a clear explanation of why this rule is being ignored. Consider adding more context about what dependencies might be missing and why they're intentionally omitted.- // biome-ignore lint/correctness/useExhaustiveDependencies: from Bazza UI + // biome-ignore lint/correctness/useExhaustiveDependencies: We only want to recreate the debounced function + // when debounceMs or onChange changes, not when the debounce function's internal implementation changes
31-35: Potential optimization for handleChangeConsider memoizing the
handleChangefunction withuseCallbackto prevent unnecessary rerenders in child components that might receive this function as a prop.- const handleChange = (e: ChangeEvent<HTMLInputElement>) => { + const handleChange = useCallback((e: ChangeEvent<HTMLInputElement>) => { const newValue = e.target.value; setValue(newValue); // Update local state immediately debouncedOnChange(newValue); // Call debounced version - }; + }, [debouncedOnChange]);packages/components/src/ui/dialog.tsx (1)
36-56: DialogContent accessibility enhancement neededThe DialogContent implementation is comprehensive with proper positioning, animations, and a close button. The close button correctly includes an accessible label.
However, consider adding an
aria-modal="true"attribute to the DialogContent to improve screen reader announcements.<DialogPrimitive.Content data-slot="dialog-content" + aria-modal="true" className={cn( 'bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 fixed top-[50%] left-[50%] z-50 grid w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] gap-4 rounded-lg border p-6 shadow-lg duration-200 sm:max-w-lg', className, )} {...props} >packages/components/src/ui/data-table-filter/lib/helpers.ts (2)
39-54: Simplify the number range creation logicThe
createNumberRangefunction has some redundant code that can be simplified.export function createNumberRange(values: number[] | undefined) { - let a = 0 - let b = 0 - if (!values || values.length === 0) return [a, b] + + let a: number, b: number + if (values.length === 1) { a = values[0] + b = 0 } else { a = values[0] b = values[1] } const [min, max] = a < b ? [a, b] : [b, a] return [min, max] }
73-90: Optimize Map type checking performanceThe
isColumnOptionMapfunction checks all keys and all values separately, which means iterating through the Map twice. Consider combining these checks for better performance.export function isColumnOptionMap( value: unknown, ): value is Map<string, number> { if (!(value instanceof Map)) { return false } - for (const key of value.keys()) { - if (typeof key !== 'string') { - return false - } - } - for (const val of value.values()) { - if (typeof val !== 'number') { - return false - } - } + for (const [key, val] of value.entries()) { + if (typeof key !== 'string' || typeof val !== 'number') { + return false + } + } return true }packages/components/src/ui/data-table-filter/components/active-filters.tsx (2)
83-90: Add an accessible label to the delete-filter button.The “X” icon alone is not screen-reader friendly. Provide
aria-label(ortitle) so assistive technologies announce the action.<Button variant="ghost" className="rounded-none rounded-r-2xl text-xs w-7 h-full" - onClick={() => actions.removeFilter(filter.columnId)} + onClick={() => actions.removeFilter(filter.columnId)} + aria-label={t('removeFilter', locale) /* e.g. “Remove filter” */} >
145-153: ThrottleonScrollhandler to avoid excessive re-renders.
checkScrollruns on every scroll event which can fire dozens of times per frame, causing frequentsetStatecalls.
Consider wrappingcheckScrollin a smallrequestAnimationFrame/throttlewrapper or the newuseDebounceCallbackhook to improve scrolling performance on low-end devices.packages/components/src/ui/data-table-filter/hooks/use-debounce-callback.tsx (1)
17-23: Avoidanyin generic constraints.
(...args: any)loses all type-safety which this hook tries to preserve.-export type DebouncedState<T extends (...args: any) => ReturnType<T>> = +export type DebouncedState<T extends (...args: unknown[]) => ReturnType<T>> =Repeat for other occurrences.
This aligns with the Biome lint error and preserves inference while still being permissive.🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 22-22: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/components/filter-operator.tsx (2)
230-239: Add heading for Date operator list for consistency & accessibility.All other controllers set
heading={t('operators', locale)}on theirCommandGroup; the Date controller omits it, so screen-readers and visual users lose contextual information.- return ( - <CommandGroup> + return ( + <CommandGroup heading={t('operators', locale)}>
288-296: UnifyonSelectsignature across controllers.Number controller wraps
changeOperatorin an extra arrow, unlike the others.
It’s harmless but inconsistent and incurs an unnecessary closure per render.- <CommandItem onSelect={() => changeOperator(r.value)} value={r.value} key={r.value}> + <CommandItem onSelect={changeOperator} value={r.value} key={r.value}>packages/components/src/ui/data-table-filter/hooks/use-data-table-filters.tsx (1)
326-329: Removeanycast by tightening generics.
values: newValues as anybreaks type-safety and hides potential bugs.
SincenewValuesis already narrowed by branch logic, you can let TS infer without casting:- values: newValues as any, + values: newValues,If the compiler still complains, consider parameterising
setFilterValuewith a concreteTTypeso inference succeeds.🧰 Tools
🪛 Biome (1.9.4)
[error] 328-328: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/remix-hook-form/data-table-router-parsers.ts (1)
12-40: Minor: tighten the runtime validator to make intent explicit
parseBazzaFiltersStatemutates an accumulator array insidereduce.
An immutable approach is a bit clearer and avoids accidental mutation bugs:- return value.reduce((acc: BazzaFiltersState, item) => { - if ( … all checks … ) { - acc.push({ …mappedFields }); - } - return acc; - }, []); + return value + .filter( + (item): item is BazzaFilterItem => + typeof item === 'object' && + item !== null && + typeof (item as any).columnId === 'string' && + typeof (item as any).type === 'string' && + typeof (item as any).operator === 'string' && + Array.isArray((item as any).values), + ) + .map((item) => ({ + columnId: item.columnId, + type: item.type, + operator: item.operator, + values: item.values, + }));Functionality stays identical but the intent (filter-then-map) is clearer and side-effect free.
packages/components/src/remix-hook-form/data-table-router-form.tsx (2)
108-131:getFilteredRowModel+manualFiltering: true= wasted CPUReact-Table will still run its filter pipeline on the client even though you always read server-filtered data. Remove the model getters you don’t need:
- getFilteredRowModel: getFilteredRowModel(), - getFacetedRowModel: getFacetedRowModel(), - getFacetedUniqueValues: getFacetedUniqueValues(),This cuts render time on wide/long tables.
165-183: Reset handler may race with URL-state syncCalling both
dtfActions.setFiltersState([])andsetUrlState({ filters: [] })can create a race
where the second call is cancelled by the first effect. Consider a single source of truth:const handleReset = () => setUrlState(standardStateValues); // URL drives Bazza via the sync-effectLess code, fewer surprises.
apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx (5)
350-356: StaleURLSearchParamscaptured in the handler
searchParamsis created once per render, but the pagination handler is memo-free, so after navigation it may mutate an out-of-date params instance, dropping newer query keys.Refactor the handlers to build a fresh
URLSearchParamsfromlocation.searchjust before mutation:-const handlePaginationChange: OnChangeFn<PaginationState> = (updaterOrValue) => { - const next = typeof updaterOrValue === 'function' ? updaterOrValue(pagination) : updaterOrValue; - searchParams.set('page', next.pageIndex.toString()); - searchParams.set('pageSize', next.pageSize.toString()); - navigate(`${location.pathname}?${searchParams.toString()}`, { replace: true }); -}; +const handlePaginationChange: OnChangeFn<PaginationState> = (updaterOrValue) => { + const next = typeof updaterOrValue === 'function' ? updaterOrValue(pagination) : updaterOrValue; + const params = new URLSearchParams(location.search); + params.set('page', String(next.pageIndex)); + params.set('pageSize', String(next.pageSize)); + navigate(`${location.pathname}?${params.toString()}`, { replace: true }); +};
358-368: Duplicate issue – sorting handlerSame stale-params risk as above; rebuild
URLSearchParamsinside the handler.
(Apply the same pattern shown in the previous diff.)
306-309: Avoid double parsing –Datealready is aDate
row.getValue('createdDate')returns the originalDate; wrapping it withnew Date()is unnecessary and silently converts invalid dates toInvalid Date.-cell: ({ row }) => <div>{new Date(row.getValue('createdDate')).toLocaleDateString()}</div>, +cell: ({ row }) => { + const date = row.getValue<Date>('createdDate'); + return <div>{date.toLocaleDateString()}</div>; +},
437-448: Remove debugconsole.logstatements before committingLeaving console output in loader code clutters browser/dev-tools and leaks internal details to users.
Please drop or guard them behind a debug flag.
579-582: Make Storybook assertion deterministic
expect(rows.length).toBeGreaterThan(1)is brittle—if the pageSize ever changes to 0 the test still passes.
Prefer asserting the exact expected count:-// First row is header, so we expect pageSize + 1 rows -expect(rows.length).toBeGreaterThan(1); +// Header row + 10 data rows (default pageSize = 10) +expect(rows).toHaveLength(11);packages/components/src/ui/data-table-filter/core/filters.ts (1)
53-57: Tightenicon()builder typingUsing
anydefeats the purpose of the strong generic API and triggers Biome errors.
Accept exactly what will be stored inColumnConfig['icon'].- icon(value: any): ColumnConfigBuilder<TData, TType, TVal, TId> { + icon(value: LucideIcon | ElementType<unknown> | ReactElement): ColumnConfigBuilder<TData, TType, TVal, TId> {
LucideIconis already used in the final config type; mirroring it here removes the explicit-anyviolations.🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/core/types.ts (1)
29-31: AvoidanyiniconpropWe can keep flexibility without sacrificing type-safety by using
unknowninstead ofany.- // biome-ignore lint/suspicious/noExplicitAny: any for flexibility - icon?: ReactElement | ElementType<any>; + icon?: ReactElement | ElementType<unknown>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (37)
.vscode/settings.json(1 hunks)apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx(1 hunks)apps/docs/src/remix-hook-form/data-table-router-form.stories.tsx(7 hunks)packages/components/package.json(1 hunks)packages/components/src/remix-hook-form/data-table-router-form.tsx(3 hunks)packages/components/src/remix-hook-form/data-table-router-parsers.ts(4 hunks)packages/components/src/remix-hook-form/data-table-router-toolbar.tsx(1 hunks)packages/components/src/ui/button.tsx(2 hunks)packages/components/src/ui/calendar.tsx(1 hunks)packages/components/src/ui/checkbox.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/active-filters.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/data-table-filter.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/filter-actions.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/filter-operator.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/filter-selector.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/filter-subject.tsx(1 hunks)packages/components/src/ui/data-table-filter/components/filter-value.tsx(1 hunks)packages/components/src/ui/data-table-filter/core/filters.ts(1 hunks)packages/components/src/ui/data-table-filter/core/operators.ts(1 hunks)packages/components/src/ui/data-table-filter/core/types.ts(1 hunks)packages/components/src/ui/data-table-filter/hooks/use-data-table-filters.tsx(1 hunks)packages/components/src/ui/data-table-filter/hooks/use-debounce-callback.tsx(1 hunks)packages/components/src/ui/data-table-filter/hooks/use-unmount.tsx(1 hunks)packages/components/src/ui/data-table-filter/index.tsx(1 hunks)packages/components/src/ui/data-table-filter/lib/array.ts(1 hunks)packages/components/src/ui/data-table-filter/lib/debounce.ts(1 hunks)packages/components/src/ui/data-table-filter/lib/filter-fns.ts(1 hunks)packages/components/src/ui/data-table-filter/lib/helpers.ts(1 hunks)packages/components/src/ui/data-table-filter/lib/i18n.ts(1 hunks)packages/components/src/ui/data-table-filter/lib/memo.ts(1 hunks)packages/components/src/ui/data-table-filter/locales/en.json(1 hunks)packages/components/src/ui/debounced-input.tsx(1 hunks)packages/components/src/ui/dialog.tsx(1 hunks)packages/components/src/ui/popover.tsx(2 hunks)packages/components/src/ui/slider.tsx(1 hunks)packages/components/src/ui/tabs.tsx(1 hunks)packages/components/src/ui/utils/debounce.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (15)
packages/components/src/ui/utils/debounce.ts (1)
packages/components/src/ui/data-table-filter/lib/debounce.ts (1)
debounce(13-138)
packages/components/src/ui/button.tsx (1)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/checkbox.tsx (1)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/calendar.tsx (2)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)packages/components/src/ui/button.tsx (1)
buttonVariants(43-43)
packages/components/src/ui/data-table-filter/components/data-table-filter.tsx (6)
packages/components/src/ui/data-table-filter/core/types.ts (4)
Column(155-157)FiltersState(250-250)DataTableFilterActions(163-184)FilterStrategy(186-186)packages/components/src/ui/data-table-filter/lib/i18n.ts (1)
Locale(3-3)packages/components/src/ui/data-table-filter/index.tsx (1)
DataTableFilter(2-2)packages/components/src/ui/data-table-filter/components/filter-selector.tsx (1)
FilterSelector(30-30)packages/components/src/ui/data-table-filter/components/filter-actions.tsx (1)
FilterActions(14-14)packages/components/src/ui/data-table-filter/components/active-filters.tsx (2)
ActiveFiltersMobileContainer(94-156)ActiveFilters(27-57)
packages/components/src/ui/tabs.tsx (1)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/data-table-filter/components/filter-actions.tsx (4)
packages/components/src/ui/data-table-filter/core/types.ts (1)
DataTableFilterActions(163-184)packages/components/src/ui/data-table-filter/lib/i18n.ts (2)
Locale(3-3)t(11-13)packages/components/src/ui/button.tsx (1)
Button(36-39)packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/slider.tsx (1)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/data-table-filter/lib/helpers.ts (1)
packages/components/src/ui/data-table-filter/core/types.ts (2)
Column(155-157)ColumnOption(23-31)
packages/components/src/ui/data-table-filter/components/filter-subject.tsx (1)
packages/components/src/ui/data-table-filter/core/types.ts (2)
ColumnDataType(41-49)Column(155-157)
packages/components/src/ui/dialog.tsx (1)
packages/components/src/ui/utils/index.ts (1)
cn(4-6)
packages/components/src/ui/data-table-filter/core/operators.ts (1)
packages/components/src/ui/data-table-filter/core/types.ts (6)
ColumnDataType(41-49)FilterOperatorTarget(259-259)FilterOperators(226-232)FilterDetails(255-257)FilterTypeOperatorDetails(303-305)FilterValues(71-71)
packages/components/src/ui/data-table-filter/lib/filter-fns.ts (3)
packages/components/src/ui/data-table-filter/core/types.ts (1)
FilterModel(243-248)packages/components/src/ui/data-table-filter/lib/array.ts (1)
intersection(1-3)packages/components/src/ui/data-table-filter/core/operators.ts (1)
dateFilterOperators(136-227)
packages/components/src/ui/data-table-filter/lib/debounce.ts (1)
packages/components/src/ui/utils/debounce.ts (1)
debounce(10-25)
apps/docs/src/remix-hook-form/data-table-router-form.stories.tsx (1)
packages/components/src/remix-hook-form/data-table-router-parsers.ts (3)
dataTableRouterParsers(82-115)BazzaFiltersState(9-9)BazzaFilterItem(2-7)
🪛 Biome (1.9.4)
packages/components/src/ui/data-table-filter/lib/memo.ts
[error] 1-1: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 23-23: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/components/src/ui/data-table-filter/hooks/use-data-table-filters.tsx
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 51-51: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 51-51: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 51-51: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 328-328: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/hooks/use-debounce-callback.tsx
[error] 17-17: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 22-22: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/remix-hook-form/data-table-router-toolbar.tsx
[error] 18-18: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 20-20: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/remix-hook-form/data-table-router-form.tsx
[error] 42-42: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 44-44: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 45-45: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/lib/array.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 28-29: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 54-54: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 54-54: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 104-105: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/components/src/ui/data-table-filter/lib/debounce.ts
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 21-21: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 93-93: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/core/filters.ts
[error] 19-19: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 36-36: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 42-42: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 53-53: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 63-63: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 72-72: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 83-83: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 94-94: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 105-105: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 163-163: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 171-171: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 290-290: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 290-290: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 290-290: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 296-296: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 296-296: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 300-300: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 308-308: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 308-308: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/core/types.ts
[error] 94-94: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 112-112: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 112-112: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 112-112: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 112-112: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 118-118: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 118-118: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 118-118: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 118-118: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 155-155: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 243-243: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🔇 Additional comments (55)
.vscode/settings.json (1)
4-4: LGTM: Appropriate dictionary addition.Adding "Bazza" to the VS Code spell checker dictionary makes sense as this PR integrates Bazza UI filters throughout the codebase.
packages/components/src/ui/slider.tsx (1)
21-54: Well-implemented accessible Slider with responsive styling.The component correctly implements:
- Proper data attributes for styling and accessibility
- Responsive layout for both horizontal and vertical orientations
- Support for multiple thumbs based on values array length
- Appropriate styling for interactive states (hover, focus, disabled)
The use of the
cnutility ensures proper class composition and Tailwind compatibility.packages/components/src/ui/popover.tsx (2)
9-10: LGTM: Appropriate addition of PopoverAnchor.Exposing the PopoverAnchor component from Radix UI's primitive provides more flexibility for positioning popovers in the UI.
33-33: LGTM: Updated exports list.The export list has been correctly updated to include the new PopoverAnchor component while maintaining alphabetical order.
packages/components/src/ui/utils/debounce.ts (2)
9-10: LGTM: Appropriate linter comment.The added linter comment properly explains why
anyis used in the generic type, which provides clarity and prevents linting issues.
13-24: LGTM: Improved function style.The debounce implementation has been refactored to use an arrow function and has better code spacing, improving readability without changing functionality.
This simpler implementation focuses on the core debounce functionality, while a more comprehensive version with additional features exists in the data-table-filter library.
packages/components/src/ui/data-table-filter/lib/i18n.ts (1)
1-9: Clean and type-safe translations mapping
Thetranslationsobject andLocaletype are correctly set up for English-only support, and importingen.jsonleverages TS module support.packages/components/src/ui/data-table-filter/index.tsx (1)
1-2: Centralized exports for data-table-filter
Re-exporting the hook and component in a single entry point simplifies imports and aligns with module conventions.packages/components/src/ui/button.tsx (4)
3-3: Verify JSX runtime configuration
Removing the namespace import of React relies on the automatic JSX transform (React 17+). Ensure your tsconfig ("jsx": "react-jsx") or Babel setup is configured accordingly.
32-34: Props extend standardized HTML attributes
ExtendingButtonHTMLAttributes<HTMLButtonElement>aligns with React's built-in types and removes the need for namespace imports. Good move for tree-shaking and type clarity.
36-39: Simplify component JSX
The one-liner return maintains behavior while improving readability. Nice refactor.
43-43: ExportingbuttonVariantsfor external composition
Re-exportingbuttonVariantsenables consumers to build custom components with shared styling logic. This enhances reusability.packages/components/package.json (1)
45-57:❓ Verification inconclusive
Bump Radix UI dependencies and add new components
Ensure the updated versions and newly added packages (slider,tabs) are correctly installed and there are no version conflicts.Run this script to confirm installed versions match
package.json:
🏁 Script executed:
#!/bin/bash # Verify installed Radix UI versions npm ls @radix-ui/react-checkbox \ @radix-ui/react-dialog \ @radix-ui/react-dropdown-menu \ @radix-ui/react-label \ @radix-ui/react-popover \ @radix-ui/react-separator \ @radix-ui/react-slider \ @radix-ui/react-slot \ @radix-ui/react-tabsLength of output: 267
Verify Radix UI dependency installation and versions
Thenpm lsoutput showed an empty tree, indicating none of the Radix packages are currently installed. Please ensure you’ve installed dependencies and that the installed versions align with your packages/components/package.json (lines 45–57):
- Run your package manager’s install command (e.g.,
npm installoryarn install).- Re-verify with a depth-restricted listing:
npm ls @radix-ui/react-checkbox \ @radix-ui/react-dialog \ @radix-ui/react-dropdown-menu \ @radix-ui/react-label \ @radix-ui/react-popover \ @radix-ui/react-separator \ @radix-ui/react-slider \ @radix-ui/react-slot \ @radix-ui/react-tabs --depth=0- If packages still don’t appear, inspect your lockfile or
node_modulesto confirm the versions (^1.3.4for slider,^1.1.11for tabs, etc.) match those in package.json.packages/components/src/ui/data-table-filter/components/filter-subject.tsx (1)
1-17: Well-structured generic component for filter subject representationThis is a clean implementation of a reusable
FilterSubjectcomponent. The component provides a consistent visual representation for filter subjects with proper handling of optional icons. Good use of TypeScript generics to maintain type safety with the data table column configuration.A few notes for consideration:
- The component properly handles the case when
column.iconis undefined with thehasIconcheck.- The CSS classes provide appropriate spacing, font styling, and ensure text doesn't wrap.
- The
select-noneclass is a good UX choice to prevent accidental text selection.packages/components/src/ui/checkbox.tsx (1)
1-27: Good implementation of accessible Checkbox component using Radix UI primitivesThis Checkbox component is well-structured and follows best practices for creating accessible UI components. It properly wraps Radix UI's checkbox primitive with appropriate styling and behavior.
Some positive aspects:
- Uses the
cnutility to combine class names efficiently- Comprehensive styling covering various states (checked, focus, disabled, invalid)
- Proper support for dark mode
- Accessible styling with appropriate focus indicators
- Proper forwarding of props to the underlying Radix component
- Good use of semantic data attributes for styling and testing
packages/components/src/ui/data-table-filter/locales/en.json (1)
1-42: Comprehensive localization file with well-organized filter terminologyThis English localization file provides a thorough set of translations for the data table filtering system. The organization is logical and covers all necessary UI elements and filter operations.
Positive aspects:
- Comprehensive coverage of all filter-related terminology
- Well-structured organization with clear key naming conventions
- Consistent terminology for similar operations across different data types
- Detailed operator descriptions that are user-friendly and precise
The localization keys follow a logical hierarchical structure (e.g.,
filters.text.contains,filters.number.is), making it easy to maintain and extend. This will provide a solid foundation for adding additional languages in the future.packages/components/src/ui/calendar.tsx (1)
1-58: Great implementation of the Calendar component!This is a well-structured implementation of a Calendar component that wraps the DayPicker from react-day-picker with consistent styling. The component:
- Properly handles both single date and range selection modes
- Uses custom icon components for navigation
- Implements responsive styling with Tailwind CSS
- Maintains full configurability by spreading remaining props
The use of the
cnutility function for class merging and thebuttonVariantsfor consistent styling shows good adherence to the design system.packages/components/src/ui/data-table-filter/components/data-table-filter.tsx (1)
1-44: Well-structured responsive filter implementationThe component provides an excellent responsive design with separate mobile and desktop layouts. Good use of:
- Conditional rendering based on screen size
- Consistent prop passing to child components
- Proper typing with generics
- Default locale handling
The mobile experience includes a scrollable container for active filters, while the desktop version has a more spacious layout - demonstrating thoughtful UI/UX design.
packages/components/src/ui/debounced-input.tsx (1)
5-14: Clean implementation of DebouncedInput componentThe component provides a well-structured debounced input with appropriate TypeScript typing, default debounce time, and proper prop passing. Good separation of immediate UI updates from debounced callbacks.
packages/components/src/ui/tabs.tsx (4)
1-10: Well-structured Tabs component with appropriate accessibility attributesThe implementation correctly uses Radix UI primitives and applies the client directive for Next.js compatibility. The Tabs component has appropriate data-slot attributes for styling/targeting and sensible default styling.
12-23: TabsList component follows best practicesThe TabsList component properly wraps the Radix UI primitive with appropriate styling and semantic structure. The inline-flex container with proper padding and rounded corners provides a good foundation for tab navigation.
25-36: TabsTrigger has comprehensive state handlingThe TabsTrigger component includes detailed styling for various states (active, focus, disabled) and handles SVG icons appropriately. The extensive Tailwind classes properly implement the component design system.
38-43: TabsContent and exports are well implementedThe TabsContent component and exports follow the established pattern. The component is minimal but functional with flex-grow and no outline when focused.
packages/components/src/ui/dialog.tsx (5)
1-21: Dialog primitives correctly implementedThe basic Dialog components (Dialog, DialogTrigger, DialogPortal, DialogClose) are properly implemented with appropriate data-slot attributes for styling and targeting.
23-34: DialogOverlay animation states well configuredThe DialogOverlay component includes proper animation states for opening and closing transitions, which provides a smooth user experience.
58-76: Header and Footer components have responsive layoutThe DialogHeader and DialogFooter components are implemented with responsive layouts that adjust based on screen size. The footer's column-reverse/row layout pattern is a good practice for mobile-first design.
78-96: Title and Description components properly styledThe DialogTitle and DialogDescription components follow the established pattern with appropriate text styling that matches the design system.
98-109: Export block is comprehensive and well-orderedAll dialog components are properly exported in an organized manner.
packages/components/src/ui/data-table-filter/lib/debounce.ts (7)
1-17: Type definitions for debounce are well structuredThe control functions and options interfaces are clearly defined, providing a good API for the debounce function.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
18-27: State variables are properly initializedThe debounce function initializes all necessary state variables and correctly extracts options with sensible defaults.
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
28-49: Core utility functions well implementedThe
invokeFuncandshouldInvokeutility functions properly handle the debounce timing logic with appropriate conditions.
51-66: Timer management functions handle edge casesThe timer functions correctly manage scheduling and calculate remaining wait times. The
remainingWaitfunction properly handles themaxWaitTimeoption.
68-90: Edge handling for leading and trailing executionThe
timerExpired,leadingEdge, andtrailingEdgefunctions implement the core debounce behavior with proper handling of leading and trailing execution options.
92-116: Main debounced function has correct behaviorThe main
debouncedfunction correctly implements the debounce pattern with proper timing and execution control based on the provided options.🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
118-137: Control functions properly implementedThe control functions (
cancel,flush,isPending) provide a comprehensive API for controlling the debounced function's behavior.packages/components/src/ui/data-table-filter/lib/helpers.ts (5)
4-12: Column retrieval function has proper error handlingThe
getColumnfunction correctly finds a column by ID and throws a descriptive error if not found, which helps with debugging.
14-21: Number filter value creation handles various casesThe
createNumberFilterValuefunction correctly handles empty, single value, and range cases for number filters.
23-30: Date filter value creation has proper validationThe
createDateFilterValuefunction correctly handles valid date filter cases and throws an error for invalid inputs.
32-54: Range creation functions ensure proper orderingBoth
createDateRangeandcreateNumberRangecorrectly ensure that the returned ranges are in ascending order, which is important for filtering operations.
56-99: Type guards improve runtime type safetyThe type guard functions are well implemented and provide runtime validation for complex types, which is essential for robust filtering operations.
packages/components/src/remix-hook-form/data-table-router-form.tsx (1)
66-79:useDataTableFilterscall: sendingdataonstrategy: "server"is misleadingWhen
strategyis"server"the hook is expected to treatdataas faceted meta, not the
full table rows – otherwise the hook might memoize large datasets unnecessarily.If the rows are already filtered server-side, consider:
- data: data, + data: [], // or omit the param if optional(or rename the prop in the hook to
initialDatavsserverDatato make the intent clearer).packages/components/src/ui/data-table-filter/lib/filter-fns.ts (1)
55-63:excludevsexclude if any ofbehave identicallyBoth branches return true when no filter value overlaps.
exclude if any ofshould only exclude when at least one value matches.- case 'exclude if any of': - return !(intersection(values, filterValues).length > 0) + case 'exclude if any of': + return intersection(values, filterValues).length === 0Verify the business rule and adjust the predicate (or operators meta) accordingly.
packages/components/src/ui/data-table-filter/lib/array.ts (5)
11-18: Avoidany– preserve type–safety of helper utilities
deepHashacceptsvalue: any, which propagates loss of type information throughout the helper stack and triggers Biome’snoExplicitAnywarning.
Because this helper is only ever read-only, switching tounknown(and narrowing viatypeof/Array.isArray) maintains safety at no cost:-function deepHash(value: any, cache = new WeakMap<object, string>()): string { +function deepHash(value: unknown, cache = new WeakMap<object, string>()): string {Do the same for
deepEqualand any other helpers that currently fall back toany.
[ suggest_essential_refactor ]🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
26-34: Cache is rebuilt on every top-level call – reconsider scope
deepHashcreates a newWeakMapfor each invocation, meaning identical objects hashed in separate calls do not benefit from memoisation.
If cross-call caching is desired, lift theWeakMapout of the function:const _deepHashCache = new WeakMap<object, string>(); function deepHash(value: unknown, cache: WeakMap<object,string> = _deepHashCache) { ... }Otherwise please add a comment clarifying the intentional per-call cache so future readers don’t assume inter-call memoisation.
[ suggest_optional_refactor ]🧰 Tools
🪛 Biome (1.9.4)
[error] 28-29: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
54-86:deepEqualedge-cases: dates,NaN, and other iterablesThe current implementation:
- Treats
Dateobjects as plain objects – two equal dates (new Date('…')) will compare false.- Fails for
NaN(NaN !== NaN).- Skips
Map,Set,RegExp, typed-arrays, etc.If these utilities will be reused outside the simple filter use-case, consider delegating to a battle-tested library (
fast-deep-equal,dequal, etc.) or extend the switch with the missing branches.
[ request_verification ]🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 54-54: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
95-123:uniqstill O(n²) in worst caseAlthough a
Mapis used, the innerduplicateFoundscan is linear in the number of collisions, which for adversarial data regresses to quadratic behaviour.
SincedeepEqualis already deterministic, you can store a single representative per hash and skip the nested loop:-const seen = new Map<string, T[]>() +const seen = new Map<string, T>() ... - if (seen.has(hash)) { - const existing = seen.get(hash)! - if (!deepEqual(existing, item)) { - result.push(item) - } - } else { - seen.set(hash, item) - result.push(item) - } + if (!seen.has(hash) || !deepEqual(seen.get(hash), item)) { + seen.set(hash, item) + result.push(item) + }[ suggest_optional_refactor ]
🧰 Tools
🪛 Biome (1.9.4)
[error] 104-105: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
138-140:removeUniquses shallow comparison – breaks for objects
values.includes(v)compares by reference; for filter value objects identical–by-content but different-by-reference this will fail.Reuse the deep-equality helpers:
-export function removeUniq<T>(arr: T[], values: T[]): T[] { - return arr.filter((v) => !values.includes(v)) +export function removeUniq<T>(arr: T[], values: T[]): T[] { + return arr.filter( + (v) => !values.some((val) => deepEqual(val, v)), + ) }[ raise_critical_issue ]
apps/docs/src/remix-hook-form/data-table-router-form.stories.tsx (5)
90-98: Duplicatetype/filterTypefields – consider consolidating
BazzaFilterColumnConfigdefines bothtypeandfilterType, each seemingly holding the same discriminator ('text' | 'option' | …). Having two sources of truth invites drift. If they are indeed identical, drop one or derive the second (type→filterType as const).
[ suggest_optional_refactor ]
100-133: Hard-coded strings – prefer string literals / enumsOperator strings such as
'option','text'etc. are sprinkled throughout. Extract them into a shared union/enumeration (already present in core filter types) to obtain full type-safety and auto-completion.
[ suggest_optional_refactor ]
135-149: Remove console debugging before commitMultiple
console.log/console.warnstatements remain:bazzaFilterColumnConfigs.forEach((config) => { console.log('>>>>> config', config); ... });Leftover logging clutters Storybook output, leaks internal data to the console and can incur performance costs. Please clean them up or guard behind a dev flag.
[ suggest_nitpick ]
156-169: Duplicate runtime option validationThe second
bazzaFilterColumnConfigs.forEachloop repeats the same label-validation logic executed just above. If you need this at render time, factor it into a reusable helper; otherwise delete to avoid needless work every render.
[ suggest_nitpick ]
217-246: Default-pass branches mask unsupported operatorsWhen an unknown operator is encountered, the filter currently returns
true, effectively disabling filtering for that criterion. This can silently hide bugs. Instead, fail fast:- default: - return true; + default: + console.warn(`Unhandled operator '${operator}' for type '${type}'`); + return false;This surfaces mis-configurations during development.
[ suggest_essential_refactor ]packages/components/src/ui/data-table-filter/components/filter-value.tsx (3)
156-160:isValidElement(Icon)always false – use element, not component
isValidElementexpects a JSX element, yetIconis a component reference. The predicate will therefore always returnfalse, making the ternary redundant.-{hasIcon && (isValidElement(Icon) ? Icon : <Icon className="size-4 text-primary" />)} +{hasIcon && <Icon className="size-4 text-primary" />}Apply the same change everywhere
isValidElement(Icon)orcloneElement(Icon, …)is used. If you need to accept both a ready-made element and a component, useReact.isValidElement(icon) ? icon : <Icon … />.
[ raise_critical_issue ]
626-630:useEffectupdate condition inverted – state never refreshesThe effect only runs
setValueswhenfilter.valuesandvaluesare already equal. The intention was presumably the opposite:-if (filter?.values && filter.values.length === values.length && filter.values.every((v,i)=>v===values[i])) { - setValues(filter.values); -} +if (filter?.values && !filter.values.every((v, i) => v === values[i])) { + setValues(filter.values); +}Without this fix, external updates to the filter are ignored, causing slider & inputs to desynchronise.
[ raise_critical_issue ]
658-684: Cancel debounce before state change to avoid stale closuresGreat job cancelling pending debounced calls before switching operator/type 👏.
Minor: theactions.setFilterOperatorcall usescolumn.id, whereassetFilterValueuses the entire column object. Consider unifying (pass column object everywhere or expose operator setter that accepts the object) to keep APIs coherent.
[ suggest_nitpick ]
| return () => { | ||
| // console.log(`[memo] Calling memoized function: ${options.key}`) | ||
|
|
||
| const deps = getDeps() | ||
|
|
||
| // If no previous deps or deps have changed, recompute | ||
| if (!prevDeps || !shallowEqual(prevDeps, deps)) { | ||
| // console.log(`[memo] Cache MISS - ${options.key}`) | ||
| cachedResult = compute(deps) | ||
| prevDeps = deps | ||
| } else { | ||
| // console.log(`[memo] Cache HIT - ${options.key}`) | ||
| } | ||
|
|
||
| return cachedResult! | ||
| } |
There was a problem hiding this comment.
Remove non-null assertion and handle undefined case properly
The code uses a non-null assertion on line 23 when returning cachedResult!. This could be problematic if compute() returns undefined. Consider handling this case explicitly.
return () => {
// console.log(`[memo] Calling memoized function: ${options.key}`)
const deps = getDeps()
// If no previous deps or deps have changed, recompute
if (!prevDeps || !shallowEqual(prevDeps, deps)) {
// console.log(`[memo] Cache MISS - ${options.key}`)
cachedResult = compute(deps)
prevDeps = deps
} else {
// console.log(`[memo] Cache HIT - ${options.key}`)
}
- return cachedResult!
+ return cachedResult as TResult
}Alternatively, you could initialize cachedResult with the result of the first computation:
export function memo<TDeps extends readonly unknown[], TResult>(
getDeps: () => TDeps,
compute: (deps: TDeps) => TResult,
options: { key: string },
): () => TResult {
- let prevDeps: TDeps | undefined
- let cachedResult: TResult | undefined
+ let prevDeps: TDeps = getDeps()
+ let cachedResult: TResult = compute(prevDeps)
return () => {
// console.log(`[memo] Calling memoized function: ${options.key}`)
const deps = getDeps()
// If no previous deps or deps have changed, recompute
- if (!prevDeps || !shallowEqual(prevDeps, deps)) {
+ if (!shallowEqual(prevDeps, deps)) {
// console.log(`[memo] Cache MISS - ${options.key}`)
cachedResult = compute(deps)
prevDeps = deps
} else {
// console.log(`[memo] Cache HIT - ${options.key}`)
}
- return cachedResult!
+ return cachedResult
}
}📝 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.
| return () => { | |
| // console.log(`[memo] Calling memoized function: ${options.key}`) | |
| const deps = getDeps() | |
| // If no previous deps or deps have changed, recompute | |
| if (!prevDeps || !shallowEqual(prevDeps, deps)) { | |
| // console.log(`[memo] Cache MISS - ${options.key}`) | |
| cachedResult = compute(deps) | |
| prevDeps = deps | |
| } else { | |
| // console.log(`[memo] Cache HIT - ${options.key}`) | |
| } | |
| return cachedResult! | |
| } | |
| export function memo<TDeps extends readonly unknown[], TResult>( | |
| getDeps: () => TDeps, | |
| compute: (deps: TDeps) => TResult, | |
| options: { key: string }, | |
| ): () => TResult { | |
| let prevDeps: TDeps = getDeps() | |
| let cachedResult: TResult = compute(prevDeps) | |
| return () => { | |
| // console.log(`[memo] Calling memoized function: ${options.key}`) | |
| const deps = getDeps() | |
| if (!shallowEqual(prevDeps, deps)) { | |
| // console.log(`[memo] Cache MISS - ${options.key}`) | |
| cachedResult = compute(deps) | |
| prevDeps = deps | |
| } else { | |
| // console.log(`[memo] Cache HIT - ${options.key}`) | |
| } | |
| return cachedResult | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/lib/memo.ts around lines 9 to
24, the return statement uses a non-null assertion on cachedResult, which can
cause issues if compute() returns undefined. To fix this, remove the non-null
assertion and explicitly handle the case where cachedResult might be undefined
by initializing cachedResult with the result of the first compute call before
returning it, ensuring the function always returns a defined value or handles
undefined safely.
| export function debounce<T extends (...args: any[]) => any>( | ||
| func: T, | ||
| wait: number, | ||
| options: DebounceOptions = {}, | ||
| ): ((...args: Parameters<T>) => ReturnType<T> | undefined) & ControlFunctions { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with more specific types
The use of any in the function signature reduces type safety. Consider using generic types with constraints instead.
-export function debounce<T extends (...args: any[]) => any>(
+export function debounce<T extends (...args: unknown[]) => unknown>(
func: T,
wait: number,
options: DebounceOptions = {},
): ((...args: Parameters<T>) => ReturnType<T> | undefined) & ControlFunctions {📝 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 function debounce<T extends (...args: any[]) => any>( | |
| func: T, | |
| wait: number, | |
| options: DebounceOptions = {}, | |
| ): ((...args: Parameters<T>) => ReturnType<T> | undefined) & ControlFunctions { | |
| export function debounce<T extends (...args: unknown[]) => unknown>( | |
| func: T, | |
| wait: number, | |
| options: DebounceOptions = {}, | |
| ): ((...args: Parameters<T>) => ReturnType<T> | undefined) & ControlFunctions { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 13-13: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/lib/debounce.ts around lines 13
to 17, replace the use of 'any' in the function signature with more specific
generic types and constraints to improve type safety. Adjust the generic
parameter to better capture the function's argument and return types without
resorting to 'any', ensuring the debounce function signature is strongly typed.
| } | ||
|
|
||
| function debounced( | ||
| this: any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with more specific type for this
Using any for the this type reduces type safety. Consider using a more specific type or unknown.
- function debounced(
- this: any,
+ function debounced(
+ this: unknown,
...args: Parameters<T>
): ReturnType<T> | undefined {📝 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.
| this: any, | |
| function debounced( | |
| this: unknown, | |
| ...args: Parameters<T> | |
| ): ReturnType<T> | undefined { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/lib/debounce.ts at line 93,
replace the `this` type annotation from `any` to a more specific type that
accurately reflects the context in which the function is called, or use
`unknown` if the exact type is not known. This will improve type safety by
avoiding the overly permissive `any` type.
| // Skip if no filter value | ||
| if (!filter.values) return null; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Skip filters with empty values as well, not only undefined.
(!filter.values) prevents rendering filters whose values field is undefined, but still renders when it is an empty array ([]).
Down-stream components assume at least one value exists and may mis-render or throw.
-// Skip if no filter value
-if (!filter.values) return null;
+// Skip if no filter value(s)
+if (!filter.values || filter.values.length === 0) return null;📝 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.
| // Skip if no filter value | |
| if (!filter.values) return null; | |
| return ( | |
| // Skip if no filter value(s) | |
| if (!filter.values || filter.values.length === 0) return null; | |
| return ( |
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/components/active-filters.tsx
around lines 41 to 44, the current check only skips filters with undefined
`values` but does not skip those with empty arrays. Update the condition to also
check if `filter.values` is an empty array and skip rendering in that case to
prevent downstream components from mis-rendering or throwing errors.
| if (!filterValue || filterValue.values.length === 0) return true | ||
|
|
||
| if ( | ||
| dateFilterOperators[filterValue.operator].target === 'single' && | ||
| filterValue.values.length > 1 | ||
| ) | ||
| throw new Error('Singular operators require at most one filter value') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Runtime error guard uses wrong operator grouping
if (filterValue.operator in ['is between', 'is not between'] && ...) – same issue as above.
Apply Array.includes for correct validation or remove the guard and rely on TypeScript to
prevent invalid combinations.
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/lib/filter-fns.ts around lines
71 to 78, the runtime error guard incorrectly uses the 'in' operator to check if
filterValue.operator is in an array, which is invalid. Replace this with
Array.includes to correctly check membership, or alternatively remove this guard
and rely on TypeScript's type checking to prevent invalid operator and value
combinations.
| filterValue.operator in ['is between', 'is not between'] && | ||
| filterValue.values.length !== 2 | ||
| ) | ||
| throw new Error('Plural operators require two filter values') |
There was a problem hiding this comment.
Incorrect operator check – in checks keys, not array inclusion
filterValue.operator in ['is between', 'is not between'] always returns false because it
compares against array indices.
- filterValue.operator in ['is between', 'is not between'] &&
+ ['is between', 'is not between'].includes(filterValue.operator) &&This allows invalid value counts to slip through undetected.
📝 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.
| filterValue.operator in ['is between', 'is not between'] && | |
| filterValue.values.length !== 2 | |
| ) | |
| throw new Error('Plural operators require two filter values') | |
| // … earlier in this function … | |
| if ( | |
| ['is between', 'is not between'].includes(filterValue.operator) && | |
| filterValue.values.length !== 2 | |
| ) { | |
| throw new Error('Plural operators require two filter values') | |
| } | |
| // … later in this function … |
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/lib/filter-fns.ts around lines
80 to 83, the code incorrectly uses the 'in' operator to check if
filterValue.operator is in the array ['is between', 'is not between'], which
actually checks for keys and always returns false. Replace this with a proper
array inclusion check using Array.prototype.includes to correctly verify if
filterValue.operator matches either 'is between' or 'is not between' before
validating the length of filterValue.values.
| // --- Bazza UI Filter Setup --- | ||
| const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>>(() => columnConfigs, []); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix generic – useMemo should return an array, not a single config
columnConfigs is an array, so the generic argument needs [].
Without this, TypeScript infers DataTableColumnConfig<MockIssue> & never[], causing downstream type-errors.
-const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>>(() => columnConfigs, []);
+const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>[]>(() => columnConfigs, []);📝 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.
| // --- Bazza UI Filter Setup --- | |
| const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>>(() => columnConfigs, []); | |
| // --- Bazza UI Filter Setup --- | |
| const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>[]>(() => columnConfigs, []); |
🤖 Prompt for AI Agents (early access)
In apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx around
lines 370 to 372, the useMemo hook is typed to return a single
DataTableColumnConfig<MockIssue> instead of an array. Update the generic type
argument to DataTableColumnConfig<MockIssue>[] to correctly reflect that
columnConfigs is an array. This will fix the TypeScript type errors caused by
the incorrect generic type.
| if (column.orderFn) { | ||
| models = models.sort((m1, m2) => | ||
| column.orderFn!(m1 as ElementType<NonNullable<TVal>>, m2 as ElementType<NonNullable<TVal>>), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertions with optional chaining
The ! operator suppresses safety checks; if orderFn is missing we still call it, defeating the earlier guard.
- models = models.sort((m1, m2) =>
- column.orderFn!(m1 as ElementType<NonNullable<TVal>>, m2 as ElementType<NonNullable<TVal>>),
- );
+ models = models.sort((m1, m2) =>
+ column.orderFn?.(m1 as ElementType<NonNullable<TVal>>, m2 as ElementType<NonNullable<TVal>>) ?? 0,
+ );Apply the same pattern to transformOptionFn below (lines 167-173).
📝 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 (column.orderFn) { | |
| models = models.sort((m1, m2) => | |
| column.orderFn!(m1 as ElementType<NonNullable<TVal>>, m2 as ElementType<NonNullable<TVal>>), | |
| ); | |
| } | |
| if (column.orderFn) { | |
| models = models.sort((m1, m2) => | |
| column.orderFn?.(m1 as ElementType<NonNullable<TVal>>, m2 as ElementType<NonNullable<TVal>>) ?? 0, | |
| ); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/core/filters.ts around lines 161
to 165, replace the non-null assertion operator (!) used on column.orderFn with
optional chaining to safely call the function only if it exists, preventing
runtime errors. Similarly, update the usage of transformOptionFn in lines 167 to
173 to use optional chaining instead of non-null assertions for safer function
calls.
| export type ColumnConfig<TData, TType extends ColumnDataType = any, TVal = unknown, TId extends string = string> = { | ||
| id: TId; | ||
| accessor: TAccessorFn<TData, TVal>; | ||
| displayName: string; | ||
| icon: LucideIcon; | ||
| type: TType; | ||
| options?: TType extends OptionBasedColumnDataType ? ColumnOption[] : never; | ||
| facetedOptions?: TType extends OptionBasedColumnDataType ? Map<string, number> : never; | ||
| min?: TType extends 'number' ? number : never; | ||
| max?: TType extends 'number' ? number : never; | ||
| transformOptionFn?: TType extends OptionBasedColumnDataType ? TTransformOptionFn<TVal> : never; | ||
| orderFn?: TType extends OptionBasedColumnDataType ? TOrderFn<TVal> : never; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate default any on ColumnConfig generics
Defaulting to any leaks through the entire type system and defeats the Biome rule.
Consider a safer default:
-export type ColumnConfig<TData, TType extends ColumnDataType = any, TVal = unknown, TId extends string = string> = {
+export type ColumnConfig<
+ TData,
+ TType extends ColumnDataType = ColumnDataType,
+ TVal = ColumnDataNativeMap[TType],
+ TId extends string = string,
+> = {This removes the explicit any, improves inference for TVal, and keeps backward compatibility.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents (early access)
In packages/components/src/ui/data-table-filter/core/types.ts around lines 94 to
106, the generic parameter TType currently defaults to any, which can cause
unsafe type inference and violate the Biome rule. To fix this, remove the
default any for TType and instead provide a safer default that improves type
inference for TVal while maintaining backward compatibility. Adjust the generic
defaults accordingly to avoid leaking any through the type system.
jaruesink
changed the title
|
I see a check failed - I'm on it! 🫡 |
|
✅ Fixed failing check in this commit. The issue was that the story was importing |
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
|
I see a check failed - I'm on it! 🫡 |
…tter compatibility
|
✅ Fixed failing check in this commit. The issue was TypeScript errors in the debounce.ts file where it was using |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/components/src/remix-hook-form/data-table-router-form.tsx (2)
23-29: 🛠️ Refactor suggestionReplace
anytypes with proper type definitionsThe use of
anytype for Bazza UI components disables type checking and reduces code quality.- filterColumnConfigs: any[]; // Placeholder type for Bazza UI ColumnConfig[] - dtfOptions?: Record<string, any[]>; - dtfFacetedData?: Record<string, any>; + filterColumnConfigs: ColumnConfig[]; // Import the type from your UI lib + dtfOptions?: Record<string, readonly OptionItem[]>; + dtfFacetedData?: Record<string, FacetedMeta>;Make sure to import these types from your UI library. This will improve type safety, enable auto-completion, and resolve the Biome lint errors.
135-138:⚠️ Potential issue
window.crypto.randomUUID()will break during SSRUsing
window.crypto.randomUUID()will cause errors during server-side rendering aswindowis undefined in Node environments.- return Array.from({ length: count }, () => window.crypto.randomUUID()); + return Array.from({ length: count }, (_, i) => `skeleton-${i}`);This change generates deterministic IDs that work in both browser and server environments while maintaining the same visual result.
packages/components/src/ui/data-table-filter/lib/debounce.ts (2)
7-10: Replaceanywith more specific typesThe use of
anyin the function signature reduces type safety. Consider usingunknownor more specific generic types.-export function debounce<T extends (...args: any[]) => any>( +export function debounce<T extends (...args: unknown[]) => unknown>( fn: T, delay: number ): (...args: Parameters<T>) => void {🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
21-32: Improve type safety and remove unnecessarythisaliasingThis implementation has two issues: using
anyfor thethisparameter type and creating an unnecessary alias forthis.- return function (this: any, ...args: Parameters<T>): void { - const context = this + return function (this: unknown, ...args: Parameters<T>): void { if (timeout) { clearTimeout(timeout) } timeout = setTimeout(() => { - fn.apply(context, args) + fn.apply(this, args) timeout = null }, delay) }🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 22-22: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/components/src/remix-hook-form/data-table-router-form.tsx(3 hunks)packages/components/src/ui/data-table-filter/lib/debounce.ts(1 hunks)packages/components/src/ui/utils/filters.ts(1 hunks)packages/components/src/ui/utils/index.ts(1 hunks)packages/components/src/ui/utils/use-data-table-filters.ts(1 hunks)packages/components/src/ui/utils/use-filter-sync.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/components/src/ui/utils/use-data-table-filters.ts
- packages/components/src/ui/utils/index.ts
- packages/components/src/ui/utils/filters.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/components/src/ui/utils/use-filter-sync.ts (1)
packages/components/src/remix-hook-form/data-table-router-parsers.ts (2)
BazzaFiltersState(9-9)dataTableRouterParsers(82-115)
packages/components/src/ui/data-table-filter/lib/debounce.ts (1)
packages/components/src/ui/utils/debounce.ts (1)
debounce(10-25)
🪛 Biome (1.9.4)
packages/components/src/remix-hook-form/data-table-router-form.tsx
[error] 42-42: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 44-44: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 45-45: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/components/src/ui/data-table-filter/lib/debounce.ts
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 21-21: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 57-57: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 22-22: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 60-60: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🔇 Additional comments (7)
packages/components/src/ui/utils/use-filter-sync.ts (2)
1-13: Well-structured imports and clear documentationThe hook is properly documented with clear JSDoc comments that explain its purpose and return values. The imports are organized logically, grouping React hooks together and importing the necessary types and utilities from related modules.
14-39: LGTM! Clean implementation of URL synchronizationThe hook implementation is well-organized and follows best practices:
- It uses
useSearchParamsto read and update URL parameters- It properly serializes/deserializes filters using the parsers
- It handles empty filter state by removing the parameter
- It uses the
replaceoption withsetSearchParamsto avoid polluting browser history- The function is properly memoized with
useCallbackThis pattern will enable consistent filter state across page reloads and browser navigation.
packages/components/src/remix-hook-form/data-table-router-form.tsx (5)
1-17: Clean up of imports reflects the architecture shiftGood job updating the imports to remove unused components and add comments explaining the changes. Removing the direct use of
ColumnFilterand commenting out thezimport shows the transition from direct TanStack Table filtering to server-side filtering with Bazza UI.
66-82: Good implementation of the Bazza UI filter integrationThe integration with
useDataTableFiltersis well-structured:
- Destructuring the returned values for clarity
- Setting the strategy to 'server'
- Properly passing data, configurations, and options
- Using URL filters as the source of truth
- Updating URL state when filters change
This maintains a clear data flow and responsibility separation.
97-131: Table configuration updated for server-side filteringThe table configuration has been properly updated to work with server-side filtering:
- Removed
columnFiltersfrom state- Set
manualFilteringtotrue- Kept
getFilteredRowModelfor table structure- Removed
onColumnFiltersChangehandlerThis ensures the table correctly relies on server-filtered data while maintaining UI functionality.
154-173: Well-implemented handlers for search and filter resetThe new handlers for search changes and filter/search reset are well-implemented:
handleSearchChangeupdates URL state and resets paginationhandleResetFiltersAndSearchproperly uses Bazza UI'sremoveAllFiltersaction- Helper variables are created to track filter/search state
This maintains a clean separation between UI actions and state updates.
177-188: Proper integration with the DataTableRouterToolbarThe toolbar integration is complete with all necessary props:
- Search state and handlers
- Reset functionality
- Bazza UI filter components, state, and actions
This ensures the toolbar can fully utilize the new filtering capabilities.
| export interface DataTableRouterFormProps<TData, TValue> { | ||
| columns: ColumnDef<TData, TValue>[]; | ||
| data: TData[]; | ||
| filterableColumns?: DataTableRouterToolbarProps<TData>['filterableColumns']; | ||
| searchableColumns?: DataTableRouterToolbarProps<TData>['searchableColumns']; | ||
| columns: ColumnDef<TData, TValue>[]; // For TanStack Table display | ||
| data: TData[]; // Data from server (already filtered/sorted/paginated) | ||
| pageCount?: number; | ||
| defaultStateValues?: Partial<DataTableRouterState>; | ||
| // New prop for Bazza UI filter configurations | ||
| // This should be typed according to bazza/ui's ColumnConfig type (e.g., from createColumnConfigHelper(...).build()) | ||
| filterColumnConfigs: any[]; // Placeholder type for Bazza UI ColumnConfig[] | ||
| // Props for server-fetched options/faceted data for bazza/ui, if needed for server strategy | ||
| dtfOptions?: Record<string, any[]>; | ||
| dtfFacetedData?: Record<string, any>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Interface updated with new props but needs proper typing
The interface correctly reflects the new functionality with added props for Bazza UI integration. However, using any types loses type safety benefits.
Import and use the proper types from your UI library for these props to ensure type safety and better developer experience.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 44-44: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 45-45: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents
In packages/components/src/remix-hook-form/data-table-router-form.tsx around
lines 35 to 46, the DataTableRouterFormProps interface uses 'any' for the
filterColumnConfigs and related props, losing type safety. To fix this, import
the appropriate ColumnConfig type from the bazza/ui library and replace the
'any[]' types with these specific types. This will enforce type safety and
improve developer experience by providing accurate typings for these props.
| /** | ||
| * Debounce function for handling user input | ||
| * @param fn Function to debounce | ||
| * @param delay Delay in milliseconds | ||
| * @returns Debounced function | ||
| */ | ||
| export function debounce<T extends (...args: any[]) => any>( | ||
| fn: T, | ||
| delay: number | ||
| ): (...args: Parameters<T>) => void { | ||
| /** | ||
| * Timeout ID for the debounced function | ||
| * Using ReturnType<typeof setTimeout> instead of NodeJS.Timeout for better compatibility | ||
| */ | ||
| let timeout: ReturnType<typeof setTimeout> | null = null | ||
|
|
||
| /** | ||
| * Debounced function | ||
| * @param args Arguments to pass to the original function | ||
| */ | ||
| return function (this: any, ...args: Parameters<T>): void { | ||
| const context = this | ||
|
|
||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| } | ||
|
|
||
| timeout = setTimeout(() => { | ||
| fn.apply(context, args) | ||
| timeout = null | ||
| }, delay) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Debounce function that returns a promise | ||
| * @param fn Function to debounce | ||
| * @param delay Delay in milliseconds | ||
| * @returns Debounced function that returns a promise | ||
| */ | ||
| export function debouncePromise<T extends (...args: any[]) => Promise<any>>( | ||
| fn: T, | ||
| delay: number | ||
| ): (...args: Parameters<T>) => Promise<ReturnType<T>> { | ||
| /** | ||
| * Timeout ID for the debounced function | ||
| * Using ReturnType<typeof setTimeout> instead of NodeJS.Timeout for better compatibility | ||
| */ | ||
| let timeout: ReturnType<typeof setTimeout> | null = null | ||
|
|
||
| /** | ||
| * Debounced function that returns a promise | ||
| * @param args Arguments to pass to the original function | ||
| * @returns Promise that resolves with the result of the original function | ||
| */ | ||
| return function ( | ||
| this: any, | ||
| ...args: Parameters<T> | ||
| ): ReturnType<typeof setTimeout> { | ||
| const context = this | ||
|
|
||
| return new Promise((resolve) => { | ||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| } | ||
|
|
||
| timeout = setTimeout(() => { | ||
| resolve(fn.apply(context, args)) | ||
| timeout = null | ||
| }, delay) | ||
| }) as unknown as ReturnType<typeof setTimeout> | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating with existing debounce utility
This file introduces new debounce utilities while there's already a similar implementation in packages/components/src/ui/utils/debounce.ts. Consider either:
- Extending the existing utility to support promises
- Replacing the existing implementation with this more comprehensive one
- Consolidating both implementations to avoid duplication
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 7-7: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 21-21: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 57-57: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 22-22: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 60-60: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🤖 Prompt for AI Agents
In packages/components/src/ui/data-table-filter/lib/debounce.ts lines 1 to 74,
the debounce utilities duplicate functionality already present in
packages/components/src/ui/utils/debounce.ts. To fix this, review the existing
debounce utility and either extend it to support promise-based functions or
replace it with this more comprehensive implementation. Consolidate the code to
avoid duplication by merging the two implementations, ensuring all required
features are covered in a single utility module.
| export function debouncePromise<T extends (...args: any[]) => Promise<any>>( | ||
| fn: T, | ||
| delay: number | ||
| ): (...args: Parameters<T>) => Promise<ReturnType<T>> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with more specific types in debouncePromise
Similar to the first function, using any reduces type safety.
-export function debouncePromise<T extends (...args: any[]) => Promise<any>>(
+export function debouncePromise<T extends (...args: unknown[]) => Promise<unknown>>(
fn: T,
delay: number
): (...args: Parameters<T>) => Promise<ReturnType<T>> {📝 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 function debouncePromise<T extends (...args: any[]) => Promise<any>>( | |
| fn: T, | |
| delay: number | |
| ): (...args: Parameters<T>) => Promise<ReturnType<T>> { | |
| export function debouncePromise<T extends (...args: unknown[]) => Promise<unknown>>( | |
| fn: T, | |
| delay: number | |
| ): (...args: Parameters<T>) => Promise<ReturnType<T>> { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 41-41: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents
In packages/components/src/ui/data-table-filter/lib/debounce.ts around lines 41
to 44, the debouncePromise function uses 'any' in the type definition for the
function parameters, which reduces type safety. Replace 'any' with a more
specific generic type or constraint that accurately reflects the expected
argument types of the function 'fn' to improve type safety and maintain proper
typings throughout the debouncePromise function.
| return function ( | ||
| this: any, | ||
| ...args: Parameters<T> | ||
| ): ReturnType<typeof setTimeout> { | ||
| const context = this | ||
|
|
||
| return new Promise((resolve) => { | ||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| } | ||
|
|
||
| timeout = setTimeout(() => { | ||
| resolve(fn.apply(context, args)) | ||
| timeout = null | ||
| }, delay) | ||
| }) as unknown as ReturnType<typeof setTimeout> | ||
| } |
There was a problem hiding this comment.
Fix return type and improve implementation of debouncePromise
There are several issues with this implementation:
- Using
anyfor thethisparameter type - Unnecessary
thisaliasing - Incorrect return type - it's returning a Promise cast to
ReturnType<typeof setTimeout>
- return function (
- this: any,
+ return function (
+ this: unknown,
...args: Parameters<T>
- ): ReturnType<typeof setTimeout> {
- const context = this
+ ): Promise<ReturnType<T>> {
- return new Promise((resolve) => {
+ return new Promise<ReturnType<T>>((resolve) => {
if (timeout) {
clearTimeout(timeout)
}
timeout = setTimeout(() => {
- resolve(fn.apply(context, args))
+ resolve(fn.apply(this, args) as ReturnType<T>)
timeout = null
}, delay)
- }) as unknown as ReturnType<typeof setTimeout>
+ })
}📝 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.
| return function ( | |
| this: any, | |
| ...args: Parameters<T> | |
| ): ReturnType<typeof setTimeout> { | |
| const context = this | |
| return new Promise((resolve) => { | |
| if (timeout) { | |
| clearTimeout(timeout) | |
| } | |
| timeout = setTimeout(() => { | |
| resolve(fn.apply(context, args)) | |
| timeout = null | |
| }, delay) | |
| }) as unknown as ReturnType<typeof setTimeout> | |
| } | |
| return function ( | |
| this: unknown, | |
| ...args: Parameters<T> | |
| ): Promise<ReturnType<T>> { | |
| return new Promise<ReturnType<T>>((resolve) => { | |
| if (timeout) { | |
| clearTimeout(timeout) | |
| } | |
| timeout = setTimeout(() => { | |
| resolve(fn.apply(this, args) as ReturnType<T>) | |
| timeout = null | |
| }, delay) | |
| }) | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 60-60: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🤖 Prompt for AI Agents
In packages/components/src/ui/data-table-filter/lib/debounce.ts around lines 56
to 72, fix the debouncePromise function by removing the use of 'any' for the
'this' parameter and avoid aliasing 'this' to a separate variable. Correct the
return type to properly reflect that the function returns a Promise of the
original function's return type, not a ReturnType of setTimeout. Adjust the
implementation so the returned function returns a Promise that resolves with the
result of the debounced function without incorrect type casting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx (1)
370-372: Fix generic –useMemoshould return an array, not a single configThe returned value is an array (
columnConfigs), so the generic needs[]:-const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>>(() => columnConfigs, []); +const bazzaProcessedColumns = useMemo<DataTableColumnConfig<MockIssue>[]>(() => columnConfigs, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx(1 hunks)packages/components/src/remix-hook-form/data-table-router-parsers.ts(4 hunks)packages/components/src/remix-hook-form/use-data-table-url-state.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/src/remix-hook-form/use-data-table-url-state.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/src/remix-hook-form/data-table-router-parsers.ts
🔇 Additional comments (2)
apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx (2)
351-368: Potential state desync by mutating a staleURLSearchParamsinstance
searchParamsis instantiated once from the initiallocation.searchand then mutated inside the pagination/sorting handlers. If the location changes elsewhere (e.g. back/forward navigation) the cached instance no longer reflects the URL, causing incorrect updates or lost parameters.Safer pattern:
-const handlePaginationChange: OnChangeFn<PaginationState> = (updaterOrValue) => { - const next = typeof updaterOrValue === 'function' ? updaterOrValue(pagination) : updaterOrValue; - searchParams.set('page', next.pageIndex.toString()); - searchParams.set('pageSize', next.pageSize.toString()); - navigate(`${location.pathname}?${searchParams.toString()}`, { replace: true }); -}; +const handlePaginationChange: OnChangeFn<PaginationState> = (updaterOrValue) => { + const next = typeof updaterOrValue === 'function' ? updaterOrValue(pagination) : updaterOrValue; + const params = new URLSearchParams(location.search); + params.set('page', next.pageIndex.toString()); + params.set('pageSize', next.pageSize.toString()); + navigate({ pathname: location.pathname, search: params.toString() }, { replace: true }); +};Same applies to
handleSortingChange.
[ suggest_essential_refactor ]
424-425: VerifyDataTableAPI – passingcolumns={columns.length}looks offMost table components expect either the column definitions or nothing at all; supplying the count may break sizing or otherwise mis-configure the component. If
DataTablereally needs the number of columns for grid measurement, rename the prop (columnCount?) to avoid confusion.Please double-check the component contract and update accordingly.
| import { type LoaderFunctionArgs, useLoaderData, useLocation, useNavigate } from 'react-router'; // Added LoaderFunctionArgs, useLoaderData, useNavigate, useLocation | ||
| import { withReactRouterStubDecorator } from '../lib/storybook/react-router-stub'; // FIX: Add withReactRouterStubDecorator |
There was a problem hiding this comment.
Incorrect router import will break at runtime/build
useLoaderData, useNavigate, and useLocation are exported from react-router-dom (or @remix-run/react when using Remix), not from react-router. Importing from the wrong package will cause TypeScript errors and, at runtime, the hooks will be undefined, crashing Storybook.
-import { type LoaderFunctionArgs, useLoaderData, useLocation, useNavigate } from 'react-router';
+import { type LoaderFunctionArgs } from '@remix-run/router'; // <-- data APIs
+import { useLoaderData, useLocation, useNavigate } from 'react-router-dom'; // <-- hooksPlease adjust the paths (and any tsconfig alias) accordingly.
📝 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 { type LoaderFunctionArgs, useLoaderData, useLocation, useNavigate } from 'react-router'; // Added LoaderFunctionArgs, useLoaderData, useNavigate, useLocation | |
| import { withReactRouterStubDecorator } from '../lib/storybook/react-router-stub'; // FIX: Add withReactRouterStubDecorator | |
| import { type LoaderFunctionArgs } from '@remix-run/router'; // <-- data APIs | |
| import { useLoaderData, useLocation, useNavigate } from 'react-router-dom'; // <-- hooks | |
| import { withReactRouterStubDecorator } from '../lib/storybook/react-router-stub'; // FIX: Add withReactRouterStubDecorator |
🤖 Prompt for AI Agents
In apps/docs/src/remix-hook-form/data-table-bazza-filters.stories.tsx at lines
24-25, the hooks useLoaderData, useNavigate, and useLocation are incorrectly
imported from 'react-router' instead of 'react-router-dom' or
'@remix-run/react'. To fix this, change the import source for these hooks to the
correct package, ensuring the imports come from 'react-router-dom' or
'@remix-run/react' as appropriate for the project setup, so the hooks are
properly defined and avoid runtime errors.
…c-functional-updates
Summary by CodeRabbit
New Features
Improvements
@radix-ui/react-*dependencies and added new ones for improved functionality.Bug Fixes
Documentation
Chores