feat: RFC DataView component#752
Merged
rohanchkrabrty merged 7 commits intomainfrom May 8, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces DataView, a comprehensive component system that unifies data presentation across multiple render formats. The implementation includes a centralized root component managing filtering, sorting, grouping, searching, and column visibility; pluggable renderers (list, table, virtualized, and custom layouts); reusable toolbar controls (search, filters, display settings); type-safe query and field definitions; and utility functions for state transformation. An RFC document specifies the architecture, rationale, and migration path from DataTable. An example page demonstrates the system with a people directory showing tabular, list, and custom card layouts alongside full query capabilities. Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
packages/raystack/components/data-view/data-view.module.css-1-20 (1)
1-20:⚠️ Potential issue | 🟡 MinorStylelint
declaration-empty-line-beforeviolations.
- Line 5: remove the empty line before
border-bottominside.toolbar.- Line 18: add an empty line before
padding(after the--select-widthcustom property) in.display-popover-properties-container.✏️ Proposed fix
.toolbar { padding: var(--rs-space-3) 0; align-self: stretch; - border-bottom: 0.5px solid var(--rs-color-border-base-primary); background: var(--rs-color-background-base-primary); } @@ .display-popover-properties-container { /* Todo: var does not exist for 160px */ --select-width: 160px; + padding: var(--rs-space-5); border-bottom: 1px solid var(--rs-color-border-base-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/data-view.module.css` around lines 1 - 20, Fix Stylelint declaration-empty-line-before violations in data-view.module.css: inside the .toolbar block (symbol .toolbar) remove the blank line before the border-bottom declaration so there is no empty line between align-self and border-bottom; inside .display-popover-properties-container (symbol .display-popover-properties-container) add a single empty line after the custom property --select-width and before the padding declaration so there is exactly one blank line between them to satisfy the rule.packages/raystack/components/data-view/hooks/useDataView.tsx-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorError message references nonexistent
DataView.Provider.The context is supplied by rendering
<DataView>(seepackages/raystack/components/data-view/data-view.tsxlines 197–230), not aDataView.Providersub-component. The message will mislead consumers debugging the error.✏️ Suggested wording
- throw new Error('useDataView must be used inside of a DataView.Provider'); + throw new Error('useDataView must be used inside of a <DataView> component');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/hooks/useDataView.tsx` around lines 8 - 10, The thrown Error in the useDataView hook references a nonexistent "DataView.Provider"; update the message to refer to the actual provider component (e.g., "<DataView>") so consumers aren't misled: find the useDataView function where ctx is checked (variable ctx) and replace the Error text to something like "useDataView must be used inside of a <DataView> component" or similar clear wording.packages/raystack/components/data-view/components/toolbar.tsx-19-47 (1)
19-47:⚠️ Potential issue | 🟡 Minor
shouldShowFiltersgates the entire Toolbar, hiding user-provided children too.When a consumer composes their own toolbar (e.g.,
<Toolbar><Search/>...other...</Toolbar>), the whole container is suppressed ifshouldShowFiltersis false. That flag is conceptually about whether to show the default filter chips; it shouldn't suppress arbitrary children like Search or custom controls. Also, the two render branches duplicate the wrappingFlex.♻️ Suggested consolidation
- const { shouldShowFilters = false } = useDataView<TData>(); - - if (!shouldShowFilters) { - return null; - } - - // If children are provided, render them so consumers can compose Search / Filters / DisplayControls. - if (children) { - return ( - <Flex - className={cx(styles['toolbar'], className)} - justify='between' - align='center' - > - {children} - </Flex> - ); - } - - return ( - <Flex - className={cx(styles['toolbar'], className)} - justify='between' - align='center' - > - <Filters<TData> /> - <DisplaySettings<TData> /> - </Flex> - ); + const { shouldShowFilters = false } = useDataView<TData>(); + + // Only gate the default composition; honour user-provided children unconditionally. + if (!children && !shouldShowFilters) { + return null; + } + + return ( + <Flex + className={cx(styles['toolbar'], className)} + justify='between' + align='center' + > + {children ?? ( + <> + <Filters<TData> /> + <DisplaySettings<TData> /> + </> + )} + </Flex> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/components/toolbar.tsx` around lines 19 - 47, The Toolbar currently returns null when useDataView().shouldShowFilters is false which also hides consumer-provided children; change Toolbar (in toolbar.tsx) to stop gating the whole component on shouldShowFilters: always render the Flex wrapper (cx(styles['toolbar'], className), justify='between', align='center') when children exist so custom Search/controls are not suppressed, and consolidate the duplicate Flex branches into a single return; for the default content, only render <Filters<TData> /> when shouldShowFilters is true and always render <DisplaySettings<TData> /> as before when there are no children.packages/raystack/components/data-view/hooks/useFilters.tsx-18-24 (1)
18-24:⚠️ Potential issue | 🟡 MinorDefault value for
multiselectfilter type falls through to''.For
FilterType.multiselect, operators arein/notinwhich expect an array value, but the ternary falls through to the empty-string branch. This will yield an initial filter withvalue: ''that downstream operator handling (notin/in) will likely mis-evaluate or crash on.🛡️ Suggested fix
const defaultValue = field.defaultFilterValue ?? (filterType === FilterType.date ? new Date() - : filterType === FilterType.select - ? options[0]?.value - : ''); + : filterType === FilterType.select + ? options[0]?.value + : filterType === FilterType.multiselect + ? [] + : '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/hooks/useFilters.tsx` around lines 18 - 24, The defaultValue ternary in useFilters.tsx incorrectly falls through FilterType.multiselect to an empty string; change the expression that computes defaultValue (the variable named defaultValue) to handle FilterType.multiselect explicitly and return an array (e.g., [] or a sensible default from options like [options[0]?.value] or options.map(...)) instead of ''. Update the branch that checks filterType so FilterType.multiselect yields an array value compatible with the 'in'/'notin' operators, leaving existing handling for FilterType.date and FilterType.select unchanged.apps/www/src/app/examples/dataview/page.tsx-515-520 (1)
515-520:⚠️ Potential issue | 🟡 MinorFix the stale DataView example link.
This page is served from
/examples/dataview, but the sidebar links the DataView item to/examples/dataview-list, which looks stale and can navigate users away from the current example.🔗 Proposed fix
<Sidebar.Item - href='/examples/dataview-list' + href='/examples/dataview' leadingIcon={<SidebarIcon />} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/app/examples/dataview/page.tsx` around lines 515 - 520, Sidebar.Item for the DataView example is linking to the stale path '/examples/dataview-list'; update the href in the Sidebar.Item that renders "DataView · People" (the Sidebar.Item with leadingIcon={<SidebarIcon />}) to point to the current page path '/examples/dataview' so the sidebar does not navigate away from the served example.packages/raystack/components/data-view/components/filters.tsx-81-154 (1)
81-154:⚠️ Potential issue | 🟡 MinorWire
classNames.addFilteror remove it from the public props.
classNames.addFilteris exposed but never applied, so consumers cannot style the add-filter control as advertised.🎨 Proposed fix
interface AddFilterProps<TData> { fieldList: DataViewField<TData>[]; appliedFiltersSet: Set<string>; onAddFilter: (field: DataViewField<TData>) => void; + className?: string; children?: Trigger<TData>; } function AddFilter<TData>({ fieldList = [], appliedFiltersSet, onAddFilter, + className, children }: AddFilterProps<TData>) { @@ - return availableFilters.length > 0 ? ( + return availableFilters.length > 0 ? ( + <span className={className}> <Menu> <Menu.Trigger render={isValidElement(trigger) ? trigger : <button>{trigger}</button>} /> @@ - </Menu> + </Menu> + </span> ) : null; } @@ <AddFilter fieldList={filterableFields} appliedFiltersSet={appliedFiltersSet} onAddFilter={onAddFilter} + className={classNames?.addFilter} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/components/filters.tsx` around lines 81 - 154, The Filters component currently exposes classNames.addFilter but never uses it; either apply that class to the AddFilter control or remove it from the public props. Fix by updating the Filters function to pass classNames?.addFilter into the AddFilter component (e.g., AddFilter className or appropriate prop) so consumers can style the add-filter control, or alternatively remove addFilter from the classNames type and all references to it in Filters and its types if we don't intend to support it. Target the Filters component, the classNames.addFilter property, and the AddFilter usage to implement the change.packages/raystack/components/data-view/components/search.tsx-53-59 (1)
53-59:⚠️ Potential issue | 🟡 MinorNormalize the search input value to prevent controlled/uncontrolled switching.
value={tableQuery?.search}passesundefinedinitially, then a string after typing. Use the nullish coalescing operator to keep the input always controlled:Proposed fix
{...props} onChange={handleSearch} - value={tableQuery?.search} + value={tableQuery?.search ?? ''} onClear={handleClear} disabled={isDisabled}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/components/search.tsx` around lines 53 - 59, The Search input is switching between uncontrolled and controlled because value={tableQuery?.search} can be undefined; update the JSX in the Search component usage to always provide a string (e.g., default to an empty string) so the input remains controlled — change the value prop expression where Search is rendered (refer to Search, tableQuery?.search, handleSearch, handleClear) to use a nullish-coalescing fallback (empty string) and ensure downstream handlers accept an empty string.docs/rfcs/002-unified-dataview-component.md-421-428 (1)
421-428:⚠️ Potential issue | 🟡 MinorRemove or replace the local worktree reference.
The RFC links to
.claude/worktrees/dataview/ANALYSIS.md, which is not a stable repo document and appears to reference removed/local analysis. This will be stale for readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/rfcs/002-unified-dataview-component.md` around lines 421 - 428, The RFC currently links to a local worktree `.claude/worktrees/dataview/ANALYSIS.md`, which is not a stable repo document and should be removed or replaced; update the Helpful Links section in docs/rfcs/002-unified-dataview-component.md by deleting that local path and either replace it with a stable, committed document (e.g., an internal RFC appendix, a committed ANALYSIS.md in the repo, or a summarized bullet) or remove the entry entirely so the RFC only references public/stable resources.packages/raystack/components/data-view/components/virtualized-content.tsx-305-316 (1)
305-316:⚠️ Potential issue | 🟡 MinorRender loader rows for initial virtualized loading.
When
isLoadingis true androws.lengthis0,hasDatais true, butshowLoaderRowsis false and the virtualizer count is0, so the body renders blank instead of showing skeletons.Also applies to: 373-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/components/virtualized-content.tsx` around lines 305 - 316, The component currently sets showLoaderRows to isLoading && rows.length > 0 and passes rows.length as the virtualizer count, so when isLoading is true but rows.length === 0 (initial load with hasData true) the virtualized list is empty; change showLoaderRows to isLoading && (rows.length > 0 || hasData) and pass a non‑zero fallback count to useVirtualizer (e.g., useVirtualizer count = showLoaderRows ? defaultSkeletonCount : rows.length) so the virtualizer will render skeleton rows during initial loading; ensure estimateSize and the row rendering logic handle those loader rows by treating them as regular rows with rowHeight or groupHeaderHeight as needed.packages/raystack/components/data-view/utils/index.tsx-120-146 (1)
120-146:⚠️ Potential issue | 🟡 MinorAvoid reference equality for filter values.
Line 144 compares raw values with
!==, so recreated arrays/objects such as multi-selectinvalues will look changed even when semantically identical.Proposed fix
+const normalizeFilterValue = (value: unknown): string => { + if (Array.isArray(value)) { + return JSON.stringify([...value].sort()); + } + return JSON.stringify(value); +}; + const generateFilterMap = ( filters: InternalFilter[] = [] -): Map<string, any> => { +): Map<string, string> => { return new Map( filters ?.filter(data => data._type === FilterType.select || data.value !== '') - .map(({ name, operator, value }) => [`${name}-${operator}`, value]) + .map(({ name, operator, value }) => [ + `${name}-${operator}`, + normalizeFilterValue(value) + ]) ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/utils/index.tsx` around lines 120 - 146, The isFilterChanged function incorrectly uses reference equality when comparing filter values; replace the raw !== comparison with a deep equality check (e.g., lodash/isEqual or a small deepEqual helper) so arrays/objects (like multi-select "in" values) compare by content; update isFilterChanged to import/use isEqual (or implement deepEqual) and change the comparison in the [...newFilterMap].some callback from oldFilterMap.get(key) !== value to !isEqual(oldFilterMap.get(key), value), ensuring null/undefined cases are handled consistently.
🧹 Nitpick comments (3)
packages/raystack/components/data-view/components/grouping.tsx (1)
29-32: Minor: search the filteredgroupableFieldsinstead offields.Since the
Selectonly exposesgroupableFields+ the default option, searchingfieldsworks today, but tightening the scope makes the invariant self-evident and prevents accidentally accepting a non-groupable accessor if a caller invokes the handler directly.- const field = fields.find(f => f.accessorKey === fieldAccessor); + const field = groupableFields.find(f => f.accessorKey === fieldAccessor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/components/grouping.tsx` around lines 29 - 32, In the grouping.tsx handler the lookup uses fields.find(...) but should search the filtered groupableFields to enforce the Select's invariant; update the lookup to use groupableFields.find(f => f.accessorKey === fieldAccessor) and then call onChange(field.accessorKey) only when that found item exists (preserving the existing conditional); this ensures fieldAccessor is validated against groupableFields rather than the full fields list (refer to symbols: fieldAccessor, fields, groupableFields, onChange).packages/raystack/components/data-view/data-view.module.css (1)
11-17: TODO: missing design tokens for300px/160px.Two
Todo: var does not existcomments hard-code values that should round-trip through design tokens. Worth tracking so the component stays consistent once tokens are added.Want me to open an issue to add the missing spacing tokens and swap these hard-coded values?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/data-view.module.css` around lines 11 - 17, Replace the hard-coded sizing with design-token CSS variables: change the min-width: 300px occurrence to use the appropriate spacing token var(--space-??) (or var(--token-spacing-300) if your token naming differs) with a pixel fallback like min-width: var(--token-spacing-300, 300px), and change --select-width: 160px inside .display-popover-properties-container to use the matching token var(--token-spacing-160, 160px); remove the "Todo: var does not exist" comments and, if the tokens don't yet exist, open an issue to add spacing tokens for these sizes and reference the selectors/minified properties (min-width and --select-width on .display-popover-properties-container) so the tokens can be added and updated later.packages/raystack/components/data-view/index.ts (1)
6-18: Consider using type-only re-exports for the DataView types as a best practice.All exported symbols (
DataViewField,DataViewFilter,DataViewListColumn,DataViewListProps,DataViewProps,DataViewQuery,DataViewSort,DataViewTableColumn,DataViewTableProps,InternalFilter,InternalQuery) are confirmed as type/interface declarations. While your current TypeScript configuration does not require it, usingexport typeclarifies intent and prevents accidental value usage:♻️ Proposed export cleanup
-export { +export type { DataViewField, DataViewFilter, DataViewListColumn, DataViewListProps, DataViewProps, DataViewQuery, DataViewSort, DataViewTableColumn, DataViewTableProps, InternalFilter, InternalQuery } from './data-view.types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-view/index.ts` around lines 6 - 18, All listed exports from './data-view.types' are type/interface-only; change the re-export to use type-only exports to make intent explicit and prevent accidental value imports — replace the current export list with type-only re-exports for DataViewField, DataViewFilter, DataViewListColumn, DataViewListProps, DataViewProps, DataViewQuery, DataViewSort, DataViewTableColumn, DataViewTableProps, InternalFilter, and InternalQuery (i.e., use "export type { ... }" for those symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/app/examples/dataview/page.tsx`:
- Around line 604-609: The custom grid is empty when all data is grouped because
the code uses table.getRowModel().rows.filter(r => !r.subRows?.length) which
only keeps top-level leaf rows; instead, traverse and flatten the row tree
returned by table.getRowModel().rows (iterating subRows recursively) to collect
all leaf rows before rendering. Update the renderer around
DataView.Renderer<Profile> to replace the current filter with a small helper
that walks rows and their subRows to produce a flat array of leaf rows
(referencing table.getRowModel().rows and the rows variable) and then use that
flattened list for the empty check and rendering.
In `@packages/raystack/components/data-view/components/display-settings.tsx`:
- Around line 98-103: Grouping is being passed all fields so non-groupable
options appear; update the props to pass only groupable fields by replacing the
fields prop on the Grouping component with fields?.filter(f => f.groupable) (use
the same null-safe pattern used elsewhere), keeping value
(tableQuery?.group_by?.[0] || defaultGroupOption.id) and handlers
onRemove/onChange unchanged so Grouping only receives groupable fields.
In `@packages/raystack/components/data-view/components/filters.tsx`:
- Around line 46-51: The icon-only filter trigger rendered when
appliedFiltersSet.size > 0 lacks an accessible name; update the IconButton (the
one that wraps FilterIcon in filters.tsx) to include an accessible label (e.g.,
add aria-label or title) so screen readers can announce it, e.g., a descriptive
string like "Filters applied" or "Open filters"; ensure the change is applied to
the IconButton usage that appears in the conditional branch referencing
appliedFiltersSet and FilterIcon.
In `@packages/raystack/components/data-view/components/list.tsx`:
- Around line 126-135: The virtualizer is reserving space for group header rows
that are rendered null when showGroupHeaders is false; update the virtualization
input so hidden group headers don't contribute height by either filtering them
out before calling useVirtualizer (pass a filteredRows array to count and to any
row access in estimateSize) or modify the estimateSize callback in
useVirtualizer to detect hidden headers (check row.subRows && row.subRows.length
> 0 and the showGroupHeaders flag) and return 0 for those rows; apply the same
change to the second virtualizer usage referenced around the 274-293 section so
virtualizer.getTotalSize() no longer includes the 36px gaps.
In `@packages/raystack/components/data-view/components/ordering.tsx`:
- Around line 60-72: The IconButton used as the sort-order toggle is missing an
accessible name; update the IconButton (the element using handleOrderChange and
value.order/SortOrders) to include an appropriate accessible label (e.g.,
aria-label or aria-pressed) that reflects the current state—use a dynamic label
like "Sort ascending" when value.order === SortOrders.ASC and "Sort descending"
otherwise—so screen readers can announce the button's purpose and current state
while keeping the existing onClick handler and disabled logic (columnList.length
=== 0) intact.
In `@packages/raystack/components/data-view/data-view.tsx`:
- Around line 168-176: The effect that syncs query?.search to internal state
uses an if (searchQuery) guard which skips updates when query.search becomes ''
or undefined, leaving tableQuery.search stale; change the useEffect to always
call updateTableQuery(prev => ({ ...prev, search: searchQuery })) whenever
searchQuery changes (remove the truthy check) so parent-controlled clears
propagate; locate the effect around searchQuery/useEffect/updateTableQuery in
data-view.tsx and update it to set search explicitly even for falsy values.
- Around line 162-166: Protect loadMoreData from duplicate/exhausted calls by
short-circuiting when a load is already in flight or when all rows are loaded:
inside loadMoreData check mode === 'server' and onLoadMore, then return early if
data.length >= totalRowCount; also track in-flight state (e.g., inFlightRef via
useRef or use an existing isLoading prop) and return if inFlightRef.current is
true; when calling onLoadMore, set inFlightRef.current = true, call onLoadMore()
and clear inFlightRef.current in a finally handler (or clear when an isLoading
prop flips) and ensure loadMoreData’s dependency array includes any new
refs/flags used. This uses the existing symbols loadMoreData, mode, onLoadMore,
data, and totalRowCount.
In `@packages/raystack/components/data-view/utils/filter-operations.tsx`:
- Around line 121-151: The select.eq / select.neq and multiselect.in /
multiselect.notin handlers currently only treat the special EmptyFilterValue
string as empty; update these functions to normalize both the incoming
filterValue.value and the cell value (row.getValue(columnId)) so that null and
undefined are treated as empty strings as well. Specifically, in select.eq and
select.neq, coerce filterValue.value and row.getValue(columnId) to '' when they
are null or undefined (or equal to EmptyFilterValue) before comparing; in
multiselect.in and multiselect.notin, map filterValue.value entries to '' when
they are null/undefined or equal to EmptyFilterValue and also coerce
row.getValue(columnId) to String('') for consistent includes checks. Ensure you
reference EmptyFilterValue and FilterValue normalization consistently across the
four functions.
- Around line 83-120: The date comparison functions (date.eq, date.neq, date.lt,
date.lte, date.gt, date.gte) currently call dayjs(row.getValue(columnId))
without validating the parsed row value, which makes invalid/missing row dates
behave like "now"; update each comparator to first extract const rowVal =
row.getValue(columnId), parse const parsedRow = dayjs(rowVal) and const
parsedFilter = dayjs(filterValue.date), then return false immediately if
parsedRow.isValid() is false or parsedFilter.isValid() is false; only perform
the existing isSame/isBefore/isAfter/isSameOrBefore/isSameOrAfter checks when
both parsedRow and parsedFilter are valid.
In `@packages/raystack/components/data-view/utils/index.tsx`:
- Around line 264-316: The conversion in the filters.map block (creating
internalFilters in index.tsx) is dropping typed values and metadata: only
`value` is used and `_type`/_dataType are set to undefined, which breaks
lossless round-trips and the reverse mapping in
transformToDataViewQuery/getFilterOperator. Fix by preserving the original typed
fields (`stringValue`, `numberValue`, `boolValue`) into the returned
InternalFilter (copy them through into transformedFilter when present) and
retain or propagate the filter's type metadata (`_type` and `_dataType`) from
the incoming `filter` (or accept field metadata as a parameter) instead of
setting them to undefined so transformToDataViewQuery can correctly remap
operators.
- Around line 148-168: The current change-detection is order-insensitive causing
reorders to be missed; update isSortChanged and isGroupChanged to perform
order-sensitive comparisons instead of using maps/sets: for isSortChanged
(function isSortChanged) first check lengths, then iterate index-by-index
comparing each DataViewSort's key and order in the same position to detect
priority reorders; for isGroupChanged (function isGroupChanged) after length
check iterate the newGroupBy array and compare each element to oldGroupBy at the
same index to detect reordering or changes. Ensure both functions return true if
any index differs.
- Around line 25-29: The filter callback that decides which filters to keep
currently calls dayjs(data.value).isValid() without checking for
null/undefined/empty and also drops non-date filters with value === '' causing
inconsistent behavior with query serialization; update the predicate used in the
filter (the anonymous filter function that references data._type and
FilterType.date) to first reject value === null || value === undefined || value
=== '' for date filters before calling dayjs(...).isValid(), and for
non-date/select filters use a looser check (reject only null/undefined, not
empty string) so empty-string select values are preserved consistently with the
serialization logic.
---
Minor comments:
In `@apps/www/src/app/examples/dataview/page.tsx`:
- Around line 515-520: Sidebar.Item for the DataView example is linking to the
stale path '/examples/dataview-list'; update the href in the Sidebar.Item that
renders "DataView · People" (the Sidebar.Item with leadingIcon={<SidebarIcon
/>}) to point to the current page path '/examples/dataview' so the sidebar does
not navigate away from the served example.
In `@docs/rfcs/002-unified-dataview-component.md`:
- Around line 421-428: The RFC currently links to a local worktree
`.claude/worktrees/dataview/ANALYSIS.md`, which is not a stable repo document
and should be removed or replaced; update the Helpful Links section in
docs/rfcs/002-unified-dataview-component.md by deleting that local path and
either replace it with a stable, committed document (e.g., an internal RFC
appendix, a committed ANALYSIS.md in the repo, or a summarized bullet) or remove
the entry entirely so the RFC only references public/stable resources.
In `@packages/raystack/components/data-view/components/filters.tsx`:
- Around line 81-154: The Filters component currently exposes
classNames.addFilter but never uses it; either apply that class to the AddFilter
control or remove it from the public props. Fix by updating the Filters function
to pass classNames?.addFilter into the AddFilter component (e.g., AddFilter
className or appropriate prop) so consumers can style the add-filter control, or
alternatively remove addFilter from the classNames type and all references to it
in Filters and its types if we don't intend to support it. Target the Filters
component, the classNames.addFilter property, and the AddFilter usage to
implement the change.
In `@packages/raystack/components/data-view/components/search.tsx`:
- Around line 53-59: The Search input is switching between uncontrolled and
controlled because value={tableQuery?.search} can be undefined; update the JSX
in the Search component usage to always provide a string (e.g., default to an
empty string) so the input remains controlled — change the value prop expression
where Search is rendered (refer to Search, tableQuery?.search, handleSearch,
handleClear) to use a nullish-coalescing fallback (empty string) and ensure
downstream handlers accept an empty string.
In `@packages/raystack/components/data-view/components/toolbar.tsx`:
- Around line 19-47: The Toolbar currently returns null when
useDataView().shouldShowFilters is false which also hides consumer-provided
children; change Toolbar (in toolbar.tsx) to stop gating the whole component on
shouldShowFilters: always render the Flex wrapper (cx(styles['toolbar'],
className), justify='between', align='center') when children exist so custom
Search/controls are not suppressed, and consolidate the duplicate Flex branches
into a single return; for the default content, only render <Filters<TData> />
when shouldShowFilters is true and always render <DisplaySettings<TData> /> as
before when there are no children.
In `@packages/raystack/components/data-view/components/virtualized-content.tsx`:
- Around line 305-316: The component currently sets showLoaderRows to isLoading
&& rows.length > 0 and passes rows.length as the virtualizer count, so when
isLoading is true but rows.length === 0 (initial load with hasData true) the
virtualized list is empty; change showLoaderRows to isLoading && (rows.length >
0 || hasData) and pass a non‑zero fallback count to useVirtualizer (e.g.,
useVirtualizer count = showLoaderRows ? defaultSkeletonCount : rows.length) so
the virtualizer will render skeleton rows during initial loading; ensure
estimateSize and the row rendering logic handle those loader rows by treating
them as regular rows with rowHeight or groupHeaderHeight as needed.
In `@packages/raystack/components/data-view/data-view.module.css`:
- Around line 1-20: Fix Stylelint declaration-empty-line-before violations in
data-view.module.css: inside the .toolbar block (symbol .toolbar) remove the
blank line before the border-bottom declaration so there is no empty line
between align-self and border-bottom; inside
.display-popover-properties-container (symbol
.display-popover-properties-container) add a single empty line after the custom
property --select-width and before the padding declaration so there is exactly
one blank line between them to satisfy the rule.
In `@packages/raystack/components/data-view/hooks/useDataView.tsx`:
- Around line 8-10: The thrown Error in the useDataView hook references a
nonexistent "DataView.Provider"; update the message to refer to the actual
provider component (e.g., "<DataView>") so consumers aren't misled: find the
useDataView function where ctx is checked (variable ctx) and replace the Error
text to something like "useDataView must be used inside of a <DataView>
component" or similar clear wording.
In `@packages/raystack/components/data-view/hooks/useFilters.tsx`:
- Around line 18-24: The defaultValue ternary in useFilters.tsx incorrectly
falls through FilterType.multiselect to an empty string; change the expression
that computes defaultValue (the variable named defaultValue) to handle
FilterType.multiselect explicitly and return an array (e.g., [] or a sensible
default from options like [options[0]?.value] or options.map(...)) instead of
''. Update the branch that checks filterType so FilterType.multiselect yields an
array value compatible with the 'in'/'notin' operators, leaving existing
handling for FilterType.date and FilterType.select unchanged.
In `@packages/raystack/components/data-view/utils/index.tsx`:
- Around line 120-146: The isFilterChanged function incorrectly uses reference
equality when comparing filter values; replace the raw !== comparison with a
deep equality check (e.g., lodash/isEqual or a small deepEqual helper) so
arrays/objects (like multi-select "in" values) compare by content; update
isFilterChanged to import/use isEqual (or implement deepEqual) and change the
comparison in the [...newFilterMap].some callback from oldFilterMap.get(key) !==
value to !isEqual(oldFilterMap.get(key), value), ensuring null/undefined cases
are handled consistently.
---
Nitpick comments:
In `@packages/raystack/components/data-view/components/grouping.tsx`:
- Around line 29-32: In the grouping.tsx handler the lookup uses
fields.find(...) but should search the filtered groupableFields to enforce the
Select's invariant; update the lookup to use groupableFields.find(f =>
f.accessorKey === fieldAccessor) and then call onChange(field.accessorKey) only
when that found item exists (preserving the existing conditional); this ensures
fieldAccessor is validated against groupableFields rather than the full fields
list (refer to symbols: fieldAccessor, fields, groupableFields, onChange).
In `@packages/raystack/components/data-view/data-view.module.css`:
- Around line 11-17: Replace the hard-coded sizing with design-token CSS
variables: change the min-width: 300px occurrence to use the appropriate spacing
token var(--space-??) (or var(--token-spacing-300) if your token naming differs)
with a pixel fallback like min-width: var(--token-spacing-300, 300px), and
change --select-width: 160px inside .display-popover-properties-container to use
the matching token var(--token-spacing-160, 160px); remove the "Todo: var does
not exist" comments and, if the tokens don't yet exist, open an issue to add
spacing tokens for these sizes and reference the selectors/minified properties
(min-width and --select-width on .display-popover-properties-container) so the
tokens can be added and updated later.
In `@packages/raystack/components/data-view/index.ts`:
- Around line 6-18: All listed exports from './data-view.types' are
type/interface-only; change the re-export to use type-only exports to make
intent explicit and prevent accidental value imports — replace the current
export list with type-only re-exports for DataViewField, DataViewFilter,
DataViewListColumn, DataViewListProps, DataViewProps, DataViewQuery,
DataViewSort, DataViewTableColumn, DataViewTableProps, InternalFilter, and
InternalQuery (i.e., use "export type { ... }" for those symbols).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5236f441-8be5-4af2-b4dd-9139b9987d7e
📒 Files selected for processing (25)
apps/www/src/app/examples/dataview/page.tsxdocs/rfcs/002-unified-dataview-component.mdpackages/raystack/components/data-view/components/content.tsxpackages/raystack/components/data-view/components/display-access.tsxpackages/raystack/components/data-view/components/display-properties.tsxpackages/raystack/components/data-view/components/display-settings.tsxpackages/raystack/components/data-view/components/filters.tsxpackages/raystack/components/data-view/components/grouping.tsxpackages/raystack/components/data-view/components/list.tsxpackages/raystack/components/data-view/components/ordering.tsxpackages/raystack/components/data-view/components/renderer.tsxpackages/raystack/components/data-view/components/search.tsxpackages/raystack/components/data-view/components/table.tsxpackages/raystack/components/data-view/components/toolbar.tsxpackages/raystack/components/data-view/components/virtualized-content.tsxpackages/raystack/components/data-view/context.tsxpackages/raystack/components/data-view/data-view.module.csspackages/raystack/components/data-view/data-view.tsxpackages/raystack/components/data-view/data-view.types.tsxpackages/raystack/components/data-view/hooks/useDataView.tsxpackages/raystack/components/data-view/hooks/useFilters.tsxpackages/raystack/components/data-view/index.tspackages/raystack/components/data-view/utils/filter-operations.tsxpackages/raystack/components/data-view/utils/index.tsxpackages/raystack/index.tsx
Comment on lines
+604
to
+609
| <DataView.Renderer<Profile>> | ||
| {({ table }) => { | ||
| const rows = table | ||
| .getRowModel() | ||
| .rows.filter(r => !r.subRows?.length); | ||
| if (!rows.length) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect DataView grouping usage and custom renderer row-model access patterns.
rg -n -C4 'getRowModel\(\)\.rows|subRows|showGroupHeaders|groupable' apps/www/src/app/examples packages/raystack/components/data-viewRepository: raystack/apsara
Length of output: 20638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any existing utilities that flatten rows or collect leaf rows
rg -n 'flatMap|collectLeaf|getLeaf|flatRow|flatten.*row' packages/raystack/components/data-view --type ts --type tsx
# Also check if there's a utility that extends countLeafRows
rg -n -A10 'countLeafRows|function.*Leaf' packages/raystack/components/data-view/utilsRepository: raystack/apsara
Length of output: 86
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what table.getRowModel() returns when grouping is active
# by looking at how it's used in list component
rg -n -B5 -A10 'table\.getRowModel\(\)' packages/raystack/components/data-view/components/list.tsx | head -50Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact context of the problematic code - what does the custom renderer do with rows?
cat -n apps/www/src/app/examples/dataview/page.tsx | sed -n '604,630p'Repository: raystack/apsara
Length of output: 1301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for leaf row collection patterns (adjust file type flags)
rg -n 'flatMap|collectLeaf|getLeaf|flatRow' packages/raystack/components/data-view
# Also search around countLeafRows to see if there's a companion function
rg -n -B2 -A15 'function countLeafRows' packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 1130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are exports from utils that might help with row flattening
grep -n "export.*function\|export const" packages/raystack/components/data-view/utils/index.tsx | grep -i 'row\|leaf\|flat'Repository: raystack/apsara
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the full context of how getRowModel is being used elsewhere
rg -n -B3 -A8 'getRowModel\(\)\.rows' packages/raystack/components/data-view/components/Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any function that collects/flattens rows (not just counts)
rg -n -A20 'export.*function.*row|export const.*row' packages/raystack/components/data-view/utils/index.tsx | head -80Repository: raystack/apsara
Length of output: 911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the Row type to understand the subRows structure
rg -n 'Row<T>' packages/raystack/components/data-view/utils/index.tsx | head -10
# Also check imports to see where Row comes from
head -30 packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a function to get/collect leaf rows (not just count)
rg -n 'leafRows\|getLeaf\|flatten' packages/raystack/components/data-view/utils/index.tsx
# Also check what functions are exported from utils
rg -n "^export" packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefuralkka
# Let's see how the Renderer component is used and what it expects
rg -n -B5 -A15 'DataView.Renderer' apps/www/src/app/examples/dataview/page.tsx | head -60Repository: raystack/apsara
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what functions are exported from utils
rg -n "^export" packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's see how the Renderer component is used and what it expects
rg -n -B5 -A15 'DataView.Renderer' apps/www/src/app/examples/dataview/page.tsx | head -60Repository: raystack/apsara
Length of output: 1244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's check the DataView.Renderer component definition to understand what it receives
rg -n -B5 -A20 'interface.*Renderer|type.*Renderer|Renderer.*=' packages/raystack/components/data-view/components/renderer.tsx 2>/dev/null | head -60Repository: raystack/apsara
Length of output: 1018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if renderer.tsx exists and what it contains
fd -type f 'renderer' packages/raystack/components/data-view/Repository: raystack/apsara
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the Renderer component definition in the main index or other files
rg -n 'export.*Renderer\|const Renderer' packages/raystack/components/data-view/ --type ts | head -20Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the DataViewContextType to see what table is
rg -n -B2 -A10 'DataViewContextType' packages/raystack/components/data-view/data-view.types.tsxRepository: raystack/apsara
Length of output: 571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's understand the actual behavior: check how groupData works
rg -n -B5 -A25 'export function groupData' packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 1090
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's see the complete groupData function to understand the structure better
rg -n -A30 'export function groupData' packages/raystack/components/data-view/utils/index.tsxRepository: raystack/apsara
Length of output: 1201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what happens when grouping is active - look at the table setup in data-view.tsx
rg -n -B5 -A15 'getSubRows.*row.*subRows' packages/raystack/components/data-view/data-view.tsxRepository: raystack/apsara
Length of output: 1022
Flatten grouped rows before rendering the custom grid.
When grouping is enabled, table.getRowModel().rows contains group rows with subRows; filtering out rows with subRows removes group headers but only keeps top-level leaf rows. If all data is grouped, there are no top-level leaf rows—they exist only in nested subRows arrays—causing the custom view to appear empty.
🐛 Proposed fix
<DataView.Renderer<Profile>>
{({ table }) => {
- const rows = table
- .getRowModel()
- .rows.filter(r => !r.subRows?.length);
+ const collectLeafRows = (
+ rows: ReturnType<typeof table.getRowModel>['rows']
+ ): ReturnType<typeof table.getRowModel>['rows'] =>
+ rows.flatMap(row =>
+ row.subRows?.length ? collectLeafRows(row.subRows) : [row]
+ );
+
+ const rows = collectLeafRows(table.getRowModel().rows);
if (!rows.length) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/app/examples/dataview/page.tsx` around lines 604 - 609, The
custom grid is empty when all data is grouped because the code uses
table.getRowModel().rows.filter(r => !r.subRows?.length) which only keeps
top-level leaf rows; instead, traverse and flatten the row tree returned by
table.getRowModel().rows (iterating subRows recursively) to collect all leaf
rows before rendering. Update the renderer around DataView.Renderer<Profile> to
replace the current filter with a small helper that walks rows and their subRows
to produce a flat array of leaf rows (referencing table.getRowModel().rows and
the rows variable) and then use that flattened list for the empty check and
rendering.
Comment on lines
+98
to
+103
| <Grouping | ||
| fields={fields ?? []} | ||
| onRemove={onGroupRemove} | ||
| onChange={onGroupChange} | ||
| value={tableQuery?.group_by?.[0] || defaultGroupOption.id} | ||
| /> |
Contributor
There was a problem hiding this comment.
Filter grouping options by groupable.
Grouping currently receives every field, so non-groupable fields can still be selected. Mirror the sortable filtering and pass only fields.filter(f => f.groupable).
Suggested fix
+ const groupableFields = (fields ?? []).filter(f => f.groupable);
+
return (
@@
<Grouping
- fields={fields ?? []}
+ fields={groupableFields}
onRemove={onGroupRemove}
onChange={onGroupChange}
value={tableQuery?.group_by?.[0] || defaultGroupOption.id}📝 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.
Suggested change
| <Grouping | |
| fields={fields ?? []} | |
| onRemove={onGroupRemove} | |
| onChange={onGroupChange} | |
| value={tableQuery?.group_by?.[0] || defaultGroupOption.id} | |
| /> | |
| const groupableFields = (fields ?? []).filter(f => f.groupable); | |
| <Grouping | |
| fields={groupableFields} | |
| onRemove={onGroupRemove} | |
| onChange={onGroupChange} | |
| value={tableQuery?.group_by?.[0] || defaultGroupOption.id} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/components/display-settings.tsx`
around lines 98 - 103, Grouping is being passed all fields so non-groupable
options appear; update the props to pass only groupable fields by replacing the
fields prop on the Grouping component with fields?.filter(f => f.groupable) (use
the same null-safe pattern used elsewhere), keeping value
(tableQuery?.group_by?.[0] || defaultGroupOption.id) and handlers
onRemove/onChange unchanged so Grouping only receives groupable fields.
Comment on lines
+46
to
+51
| else if (appliedFiltersSet.size > 0) | ||
| return ( | ||
| <IconButton size={4}> | ||
| <FilterIcon /> | ||
| </IconButton> | ||
| ); |
Contributor
There was a problem hiding this comment.
Add an accessible label to the icon-only filter trigger.
After filters are applied, the default trigger becomes an icon-only button without an accessible name.
♿ Proposed fix
else if (appliedFiltersSet.size > 0)
return (
- <IconButton size={4}>
+ <IconButton size={4} aria-label='Add filter'>
<FilterIcon />
</IconButton>
);📝 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.
Suggested change
| else if (appliedFiltersSet.size > 0) | |
| return ( | |
| <IconButton size={4}> | |
| <FilterIcon /> | |
| </IconButton> | |
| ); | |
| else if (appliedFiltersSet.size > 0) | |
| return ( | |
| <IconButton size={4} aria-label='Add filter'> | |
| <FilterIcon /> | |
| </IconButton> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/components/filters.tsx` around lines
46 - 51, The icon-only filter trigger rendered when appliedFiltersSet.size > 0
lacks an accessible name; update the IconButton (the one that wraps FilterIcon
in filters.tsx) to include an accessible label (e.g., add aria-label or title)
so screen readers can announce it, e.g., a descriptive string like "Filters
applied" or "Open filters"; ensure the change is applied to the IconButton usage
that appears in the conditional branch referencing appliedFiltersSet and
FilterIcon.
Comment on lines
+126
to
+135
| const virtualizer = useVirtualizer({ | ||
| count: rows.length, | ||
| getScrollElement: () => scrollRef.current, | ||
| estimateSize: i => { | ||
| const row = rows[i]; | ||
| const isGroupHeader = row?.subRows && row.subRows.length > 0; | ||
| return isGroupHeader ? 36 : rowHeight; | ||
| }, | ||
| overscan | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid reserving virtual space for hidden group headers.
With virtualized and showGroupHeaders={false}, group header rows return null but still contribute 36px to virtualizer.getTotalSize(), leaving visible blank gaps. Filter them out before virtualizing, or estimate them as 0 and ensure the virtualizer handles zero-height rows correctly.
Suggested direction
+ const rowsForRendering = useMemo(
+ () =>
+ showGroupHeaders
+ ? rows
+ : rows.filter(row => !(row.subRows && row.subRows.length > 0)),
+ [rows, showGroupHeaders]
+ );
+
const virtualizer = useVirtualizer({
- count: rows.length,
+ count: rowsForRendering.length,
@@
- const row = rows[i];
+ const row = rowsForRendering[i];
@@
- const row = rows[item.index];
+ const row = rowsForRendering[item.index];Also applies to: 274-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/components/list.tsx` around lines 126
- 135, The virtualizer is reserving space for group header rows that are
rendered null when showGroupHeaders is false; update the virtualization input so
hidden group headers don't contribute height by either filtering them out before
calling useVirtualizer (pass a filteredRows array to count and to any row access
in estimateSize) or modify the estimateSize callback in useVirtualizer to detect
hidden headers (check row.subRows && row.subRows.length > 0 and the
showGroupHeaders flag) and return 0 for those rows; apply the same change to the
second virtualizer usage referenced around the 274-293 section so
virtualizer.getTotalSize() no longer includes the 36px gaps.
Comment on lines
+60
to
+72
| <IconButton | ||
| onClick={handleOrderChange} | ||
| size={4} | ||
| disabled={columnList.length === 0} | ||
| > | ||
| {value.order === SortOrders?.ASC ? ( | ||
| <TextAlignBottomIcon | ||
| className={styles['display-popover-sort-icon']} | ||
| /> | ||
| ) : ( | ||
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | ||
| )} | ||
| </IconButton> |
Contributor
There was a problem hiding this comment.
Add an accessible label to the sort-order toggle.
This icon-only IconButton has no accessible name, so screen-reader users cannot tell what action it performs.
♿ Proposed fix
<IconButton
onClick={handleOrderChange}
size={4}
disabled={columnList.length === 0}
+ aria-label={
+ value.order === SortOrders.ASC
+ ? 'Switch to descending order'
+ : 'Switch to ascending order'
+ }
>📝 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.
Suggested change
| <IconButton | |
| onClick={handleOrderChange} | |
| size={4} | |
| disabled={columnList.length === 0} | |
| > | |
| {value.order === SortOrders?.ASC ? ( | |
| <TextAlignBottomIcon | |
| className={styles['display-popover-sort-icon']} | |
| /> | |
| ) : ( | |
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | |
| )} | |
| </IconButton> | |
| <IconButton | |
| onClick={handleOrderChange} | |
| size={4} | |
| disabled={columnList.length === 0} | |
| aria-label={ | |
| value.order === SortOrders.ASC | |
| ? 'Switch to descending order' | |
| : 'Switch to ascending order' | |
| } | |
| > | |
| {value.order === SortOrders?.ASC ? ( | |
| <TextAlignBottomIcon | |
| className={styles['display-popover-sort-icon']} | |
| /> | |
| ) : ( | |
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | |
| )} | |
| </IconButton> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/components/ordering.tsx` around lines
60 - 72, The IconButton used as the sort-order toggle is missing an accessible
name; update the IconButton (the element using handleOrderChange and
value.order/SortOrders) to include an appropriate accessible label (e.g.,
aria-label or aria-pressed) that reflects the current state—use a dynamic label
like "Sort ascending" when value.order === SortOrders.ASC and "Sort descending"
otherwise—so screen readers can announce the button's purpose and current state
while keeping the existing onClick handler and disabled logic (columnList.length
=== 0) intact.
Comment on lines
+83
to
+120
| date: { | ||
| eq: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return dayjs(row.getValue(columnId)).isSame( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| }, | ||
| neq: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return !dayjs(row.getValue(columnId)).isSame( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| }, | ||
| lt: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return dayjs(row.getValue(columnId)).isBefore( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| }, | ||
| lte: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return dayjs(row.getValue(columnId)).isSameOrBefore( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| }, | ||
| gt: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return dayjs(row.getValue(columnId)).isAfter( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| }, | ||
| gte: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| return dayjs(row.getValue(columnId)).isSameOrAfter( | ||
| dayjs(filterValue.date), | ||
| 'day' | ||
| ); | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dayjs behavior when parsing undefined or invalid date values isValid current date
💡 Result:
Day.js treats undefined as no argument, parsing to the current date and time, so dayjs(undefined).isValid returns true. In contrast, null parses to an invalid date, so dayjs(null).isValid returns false. For invalid date strings: - Non-strict parsing (default): Day.js uses lenient parsing similar to native JavaScript Date. Overflow dates like '2022-02-31' parse to a valid nearby date (e.g., 2022-03-03) and isValid returns true. Completely unparseable strings like 'some invalid string' result in invalid dates, isValid returns false. - Strict parsing: Requires the customParseFormat plugin. Use dayjs('2022-02-31', 'YYYY-MM-DD', true).isValid which returns false for invalid dates. This behavior matches Moment.js compatibility and native Date leniency. Always check .isValid after parsing, especially for user input.
Citations:
- 1: dayjs(undefined).isValid() show devHelper warn in development iamkun/dayjs#1240
- 2: https://day.js.org/docs/en/parse/is-valid
- 3: Inconsistent constructor behavior around
undefinedandnulliamkun/dayjs#361 - 4: https://www.dayjs.com/docs/en/parse/string-format
- 5: https://day.js.org/docs/en/parse/parse
- 6: https://readmex.com/en-US/iamkun/dayjs/page-4.15e248b21-f323-4dc5-9fa0-250ef2797b4e
- 7:
dayjs(null)throws error, not return dayjs object as invalid date iamkun/dayjs#2095 - 8: ambiguous definition of
isValid()iamkun/dayjs#1850 - 9: https://stackoverflow.com/questions/58216504/dayjs-isvalid-behaves-differently-to-moment
- 10: Validation .isValid() doesn't work always iamkun/dayjs#320
- 11: Invalid dates are parsed as valid iamkun/dayjs#1238
🏁 Script executed:
# Check if the file exists and examine the code around lines 83-120
file="packages/raystack/components/data-view/utils/filter-operations.tsx"
if [ -f "$file" ]; then
echo "=== File exists ==="
wc -l "$file"
echo ""
echo "=== Lines 75-130 (context around the reported lines) ==="
sed -n '75,130p' "$file"
else
echo "File not found at: $file"
# Try to find similar files
find . -name "*filter-operations*" -type f 2>/dev/null | head -20
fiRepository: raystack/apsara
Length of output: 2081
🏁 Script executed:
# Search for FilterValue type definition to understand its structure
rg "type FilterValue|interface FilterValue" --type tsx --type ts -A 5Repository: raystack/apsara
Length of output: 86
🏁 Script executed:
# Check if there are other date-related comparisons or similar patterns in the file
rg "dayjs\(" "packages/raystack/components/data-view/utils/filter-operations.tsx" -B 2 -A 2Repository: raystack/apsara
Length of output: 1311
🏁 Script executed:
# Search for FilterValue type definition
rg "type FilterValue|interface FilterValue" -A 10 --type tsRepository: raystack/apsara
Length of output: 801
🏁 Script executed:
# Look for imports and type definitions in the filter-operations file
head -50 "packages/raystack/components/data-view/utils/filter-operations.tsx"Repository: raystack/apsara
Length of output: 1916
🏁 Script executed:
# Check how filterValue is used/created for date filters - search for where date filters are called
rg "FilterType\.date|filterType.*date" --type ts -B 3 -A 3Repository: raystack/apsara
Length of output: 15975
🏁 Script executed:
# Verify if row values can be undefined or null by checking how rows are populated
rg "row.getValue|getValue\(" packages/raystack/components/data-view/utils/filter-operations.tsx -B 2 -A 2Repository: raystack/apsara
Length of output: 4220
🏁 Script executed:
# Check the actual data structure and examples to understand what values rows can have
rg "updatedAt|createdAt" packages/raystack/components/data-view -A 3 | head -50Repository: raystack/apsara
Length of output: 41
Validate row date values before comparison.
Row values passed to dayjs() are not validated; missing or invalid dates parse unexpectedly (e.g., dayjs(undefined) returns the current date as valid, causing false matches). Add validation with isValid() on the row value before comparison.
Suggested direction
+const toValidDay = (value: unknown) => {
+ const date = dayjs(value);
+ return date.isValid() ? date : null;
+};
+
export const filterOperationsMap: FilterFunctionsMap = {
@@
date: {
eq: (row, columnId, filterValue: FilterValue, _addMeta) => {
- return dayjs(row.getValue(columnId)).isSame(
- dayjs(filterValue.date),
- 'day'
- );
+ const rowDate = toValidDay(row.getValue(columnId));
+ return Boolean(rowDate && rowDate.isSame(filterValue.date, 'day'));
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/utils/filter-operations.tsx` around
lines 83 - 120, The date comparison functions (date.eq, date.neq, date.lt,
date.lte, date.gt, date.gte) currently call dayjs(row.getValue(columnId))
without validating the parsed row value, which makes invalid/missing row dates
behave like "now"; update each comparator to first extract const rowVal =
row.getValue(columnId), parse const parsedRow = dayjs(rowVal) and const
parsedFilter = dayjs(filterValue.date), then return false immediately if
parsedRow.isValid() is false or parsedFilter.isValid() is false; only perform
the existing isSame/isBefore/isAfter/isSameOrBefore/isSameOrAfter checks when
both parsedRow and parsedFilter are valid.
Comment on lines
+121
to
+151
| select: { | ||
| eq: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| if (String(filterValue.value) === EmptyFilterValue) { | ||
| return row.getValue(columnId) === ''; | ||
| } | ||
| // Select only supports string values | ||
| return String(row.getValue(columnId)) === String(filterValue.value); | ||
| }, | ||
| neq: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| if (String(filterValue.value) === EmptyFilterValue) { | ||
| return row.getValue(columnId) !== ''; | ||
| } | ||
| // Select only supports string values | ||
| return String(row.getValue(columnId)) !== String(filterValue.value); | ||
| } | ||
| }, | ||
| multiselect: { | ||
| in: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| if (!Array.isArray(filterValue.value)) return false; | ||
|
|
||
| return filterValue.value | ||
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | ||
| .includes(String(row.getValue(columnId))); | ||
| }, | ||
| notin: (row, columnId, filterValue: FilterValue, _addMeta) => { | ||
| if (!Array.isArray(filterValue.value)) return false; | ||
|
|
||
| return !filterValue.value | ||
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | ||
| .includes(String(row.getValue(columnId))); | ||
| } |
Contributor
There was a problem hiding this comment.
Treat null and undefined as empty filter values.
The empty select/multiselect checks only normalize '', so rows with null or undefined do not match an “empty” filter and may incorrectly match “not empty”.
Suggested fix
+const normalizeEmptyValue = (value: unknown) =>
+ value === null || value === undefined ? '' : String(value);
+
@@
eq: (row, columnId, filterValue: FilterValue, _addMeta) => {
if (String(filterValue.value) === EmptyFilterValue) {
- return row.getValue(columnId) === '';
+ return normalizeEmptyValue(row.getValue(columnId)) === '';
}
@@
neq: (row, columnId, filterValue: FilterValue, _addMeta) => {
if (String(filterValue.value) === EmptyFilterValue) {
- return row.getValue(columnId) !== '';
+ return normalizeEmptyValue(row.getValue(columnId)) !== '';
}
@@
.map(value => (value === EmptyFilterValue ? '' : String(value)))
- .includes(String(row.getValue(columnId)));
+ .includes(normalizeEmptyValue(row.getValue(columnId)));
@@
.map(value => (value === EmptyFilterValue ? '' : String(value)))
- .includes(String(row.getValue(columnId)));
+ .includes(normalizeEmptyValue(row.getValue(columnId)));📝 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.
Suggested change
| select: { | |
| eq: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (String(filterValue.value) === EmptyFilterValue) { | |
| return row.getValue(columnId) === ''; | |
| } | |
| // Select only supports string values | |
| return String(row.getValue(columnId)) === String(filterValue.value); | |
| }, | |
| neq: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (String(filterValue.value) === EmptyFilterValue) { | |
| return row.getValue(columnId) !== ''; | |
| } | |
| // Select only supports string values | |
| return String(row.getValue(columnId)) !== String(filterValue.value); | |
| } | |
| }, | |
| multiselect: { | |
| in: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (!Array.isArray(filterValue.value)) return false; | |
| return filterValue.value | |
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | |
| .includes(String(row.getValue(columnId))); | |
| }, | |
| notin: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (!Array.isArray(filterValue.value)) return false; | |
| return !filterValue.value | |
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | |
| .includes(String(row.getValue(columnId))); | |
| } | |
| const normalizeEmptyValue = (value: unknown) => | |
| value === null || value === undefined ? '' : String(value); | |
| select: { | |
| eq: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (String(filterValue.value) === EmptyFilterValue) { | |
| return normalizeEmptyValue(row.getValue(columnId)) === ''; | |
| } | |
| // Select only supports string values | |
| return String(row.getValue(columnId)) === String(filterValue.value); | |
| }, | |
| neq: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (String(filterValue.value) === EmptyFilterValue) { | |
| return normalizeEmptyValue(row.getValue(columnId)) !== ''; | |
| } | |
| // Select only supports string values | |
| return String(row.getValue(columnId)) !== String(filterValue.value); | |
| } | |
| }, | |
| multiselect: { | |
| in: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (!Array.isArray(filterValue.value)) return false; | |
| return filterValue.value | |
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | |
| .includes(normalizeEmptyValue(row.getValue(columnId))); | |
| }, | |
| notin: (row, columnId, filterValue: FilterValue, _addMeta) => { | |
| if (!Array.isArray(filterValue.value)) return false; | |
| return !filterValue.value | |
| .map(value => (value === EmptyFilterValue ? '' : String(value))) | |
| .includes(normalizeEmptyValue(row.getValue(columnId))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/utils/filter-operations.tsx` around
lines 121 - 151, The select.eq / select.neq and multiselect.in /
multiselect.notin handlers currently only treat the special EmptyFilterValue
string as empty; update these functions to normalize both the incoming
filterValue.value and the cell value (row.getValue(columnId)) so that null and
undefined are treated as empty strings as well. Specifically, in select.eq and
select.neq, coerce filterValue.value and row.getValue(columnId) to '' when they
are null or undefined (or equal to EmptyFilterValue) before comparing; in
multiselect.in and multiselect.notin, map filterValue.value entries to '' when
they are null/undefined or equal to EmptyFilterValue and also coerce
row.getValue(columnId) to String('') for consistent includes checks. Ensure you
reference EmptyFilterValue and FilterValue normalization consistently across the
four functions.
Comment on lines
+25
to
+29
| ?.filter(data => { | ||
| if (data._type === FilterType.date) return dayjs(data.value).isValid(); | ||
| if (data.value !== '') return true; | ||
| return false; | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In dayjs 1.11.11, what does dayjs(undefined).isValid() return?
💡 Result:
In dayjs 1.11.11, dayjs(undefined).isValid returns true.
Citations:
- 1: dayjs(undefined).isValid() show devHelper warn in development iamkun/dayjs#1240
- 2: isSame() returns true when comparing a certain date to undefined iamkun/dayjs#2568
Reject empty/undefined date values before Day.js validation.
Line 26 calls dayjs(data.value).isValid() without pre-checking for undefined, null, or '', but dayjs(undefined).isValid() returns true in version 1.11.11. This means undefined or null date filters will be incorrectly accepted as valid. Additionally, line 27 drops select filters with value === '', while line 226 preserves them unconditionally, creating a mismatch between table state filtering and query serialization.
Add explicit value checks before calling Day.js:
Proposed fix
+const hasFilterValue = (value: unknown): boolean =>
+ value !== undefined && value !== null && value !== '';
+
+const isActiveInternalFilter = (data: InternalFilter): boolean => {
+ if (data._type === FilterType.date) {
+ return hasFilterValue(data.value) && dayjs(data.value).isValid();
+ }
+ return hasFilterValue(data.value) || data._type === FilterType.select;
+};
+
export function queryToTableState(query: InternalQuery): Partial<TableState> {
const columnFilters =
query.filters
- ?.filter(data => {
- if (data._type === FilterType.date) return dayjs(data.value).isValid();
- if (data.value !== '') return true;
- return false;
- })
+ ?.filter(isActiveInternalFilter)
?.map(data => { const sanitizedFilters =
filters
- ?.filter(data => {
- if (data._type === FilterType.select) return true;
- if (data._type === FilterType.date) return dayjs(data.value).isValid();
- if (data.value !== '') return true;
- return false;
- })
+ ?.filter(isActiveInternalFilter)
?.map(data => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/utils/index.tsx` around lines 25 - 29,
The filter callback that decides which filters to keep currently calls
dayjs(data.value).isValid() without checking for null/undefined/empty and also
drops non-date filters with value === '' causing inconsistent behavior with
query serialization; update the predicate used in the filter (the anonymous
filter function that references data._type and FilterType.date) to first reject
value === null || value === undefined || value === '' for date filters before
calling dayjs(...).isValid(), and for non-date/select filters use a looser check
(reject only null/undefined, not empty string) so empty-string select values are
preserved consistently with the serialization logic.
Comment on lines
+148
to
+168
| const isSortChanged = ( | ||
| oldSort: DataViewSort[] = [], | ||
| newSort: DataViewSort[] = [] | ||
| ): boolean => { | ||
| if (oldSort.length !== newSort.length) return true; | ||
|
|
||
| const oldSortMap = generateSortMap(oldSort); | ||
| const newSortMap = generateSortMap(newSort); | ||
|
|
||
| return [...newSortMap].some(([key, order]) => oldSortMap.get(key) !== order); | ||
| }; | ||
|
|
||
| const isGroupChanged = ( | ||
| oldGroupBy: string[] = [], | ||
| newGroupBy: string[] = [] | ||
| ): boolean => { | ||
| if (oldGroupBy.length !== newGroupBy.length) return true; | ||
|
|
||
| const oldGroupSet = new Set(oldGroupBy); | ||
| return newGroupBy.some(item => !oldGroupSet.has(item)); | ||
| }; |
Contributor
There was a problem hiding this comment.
Preserve sort/group order when detecting changes.
Line 154 and Line 166 make the comparison order-insensitive. If users reorder multi-sort priority or nested grouping, hasQueryChanged can return false and skip the update.
Proposed fix
const isSortChanged = (
oldSort: DataViewSort[] = [],
newSort: DataViewSort[] = []
): boolean => {
if (oldSort.length !== newSort.length) return true;
- const oldSortMap = generateSortMap(oldSort);
- const newSortMap = generateSortMap(newSort);
-
- return [...newSortMap].some(([key, order]) => oldSortMap.get(key) !== order);
+ return oldSort.some(
+ (item, index) =>
+ item.name !== newSort[index]?.name || item.order !== newSort[index]?.order
+ );
};
const isGroupChanged = (
oldGroupBy: string[] = [],
newGroupBy: string[] = []
): boolean => {
if (oldGroupBy.length !== newGroupBy.length) return true;
- const oldGroupSet = new Set(oldGroupBy);
- return newGroupBy.some(item => !oldGroupSet.has(item));
+ return oldGroupBy.some((item, index) => item !== newGroupBy[index]);
};📝 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.
Suggested change
| const isSortChanged = ( | |
| oldSort: DataViewSort[] = [], | |
| newSort: DataViewSort[] = [] | |
| ): boolean => { | |
| if (oldSort.length !== newSort.length) return true; | |
| const oldSortMap = generateSortMap(oldSort); | |
| const newSortMap = generateSortMap(newSort); | |
| return [...newSortMap].some(([key, order]) => oldSortMap.get(key) !== order); | |
| }; | |
| const isGroupChanged = ( | |
| oldGroupBy: string[] = [], | |
| newGroupBy: string[] = [] | |
| ): boolean => { | |
| if (oldGroupBy.length !== newGroupBy.length) return true; | |
| const oldGroupSet = new Set(oldGroupBy); | |
| return newGroupBy.some(item => !oldGroupSet.has(item)); | |
| }; | |
| const isSortChanged = ( | |
| oldSort: DataViewSort[] = [], | |
| newSort: DataViewSort[] = [] | |
| ): boolean => { | |
| if (oldSort.length !== newSort.length) return true; | |
| return oldSort.some( | |
| (item, index) => | |
| item.name !== newSort[index]?.name || item.order !== newSort[index]?.order | |
| ); | |
| }; | |
| const isGroupChanged = ( | |
| oldGroupBy: string[] = [], | |
| newGroupBy: string[] = [] | |
| ): boolean => { | |
| if (oldGroupBy.length !== newGroupBy.length) return true; | |
| return oldGroupBy.some((item, index) => item !== newGroupBy[index]); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/utils/index.tsx` around lines 148 -
168, The current change-detection is order-insensitive causing reorders to be
missed; update isSortChanged and isGroupChanged to perform order-sensitive
comparisons instead of using maps/sets: for isSortChanged (function
isSortChanged) first check lengths, then iterate index-by-index comparing each
DataViewSort's key and order in the same position to detect priority reorders;
for isGroupChanged (function isGroupChanged) after length check iterate the
newGroupBy array and compare each element to oldGroupBy at the same index to
detect reordering or changes. Ensure both functions return true if any index
differs.
Comment on lines
+264
to
+316
| const internalFilters: InternalFilter[] = filters.map(filter => { | ||
| const { | ||
| operator, | ||
| value, | ||
| stringValue, | ||
| numberValue, | ||
| boolValue, | ||
| ...filterRest | ||
| } = filter; | ||
|
|
||
| // Reverse the operator mapping and wildcard transformation | ||
| let transformedFilter = { | ||
| operator: operator as FilterOperatorTypes, | ||
| value: value, | ||
| ...(stringValue !== undefined && { stringValue }), | ||
| ...(numberValue !== undefined && { numberValue }), | ||
| ...(boolValue !== undefined && { boolValue }) | ||
| }; | ||
|
|
||
| // If operator is 'ilike', determine the original operator based on wildcards | ||
| if (operator === 'ilike' && stringValue) { | ||
| if (stringValue.startsWith('%') && stringValue.endsWith('%')) { | ||
| transformedFilter = { | ||
| operator: 'contains', | ||
| value: stringValue.slice(1, -1) // Remove % from both ends | ||
| }; | ||
| } else if (stringValue.endsWith('%')) { | ||
| transformedFilter = { | ||
| operator: 'starts_with', | ||
| value: stringValue.slice(0, -1) // Remove % from end | ||
| }; | ||
| } else if (stringValue.startsWith('%')) { | ||
| transformedFilter = { | ||
| operator: 'ends_with', | ||
| value: stringValue.slice(1) // Remove % from start | ||
| }; | ||
| } else { | ||
| // Default to contains if no wildcards (shouldn't happen normally) | ||
| transformedFilter = { | ||
| operator: 'contains', | ||
| value: stringValue | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...filterRest, | ||
| ...transformedFilter, | ||
| // We don't have type information, so leave it undefined | ||
| // The UI will need to infer or set these based on column definitions | ||
| _type: undefined, | ||
| _dataType: undefined | ||
| } as InternalFilter; |
Contributor
There was a problem hiding this comment.
Make DataViewQuery → InternalQuery round-trips lossless.
Line 277 only uses filter.value; filters represented by stringValue, numberValue, or boolValue can become value: undefined. Lines 312-315 also erase type metadata, so later transformToDataViewQuery calls may emit invalid string operators instead of remapping through getFilterOperator.
Proposed fix direction
- let transformedFilter = {
+ const internalValue = value ?? stringValue ?? numberValue ?? boolValue;
+
+ let transformedFilter = {
operator: operator as FilterOperatorTypes,
- value: value,
+ value: internalValue,
...(stringValue !== undefined && { stringValue }),
...(numberValue !== undefined && { numberValue }),
...(boolValue !== undefined && { boolValue })
};Also pass field metadata into this conversion, or otherwise persist _type/_dataType, before returning the InternalFilter; leaving both as undefined breaks the reverse mapping used by packages/raystack/components/data-view/utils/filter-operations.tsx:228-252.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-view/utils/index.tsx` around lines 264 -
316, The conversion in the filters.map block (creating internalFilters in
index.tsx) is dropping typed values and metadata: only `value` is used and
`_type`/_dataType are set to undefined, which breaks lossless round-trips and
the reverse mapping in transformToDataViewQuery/getFilterOperator. Fix by
preserving the original typed fields (`stringValue`, `numberValue`, `boolValue`)
into the returned InternalFilter (copy them through into transformedFilter when
present) and retain or propagate the filter's type metadata (`_type` and
`_dataType`) from the incoming `filter` (or accept field metadata as a
parameter) instead of setting them to undefined so transformToDataViewQuery can
correctly remap operators.
rohanchkrabrty
changed the title
rohanchkrabrty
requested review from
paanSinghCoder,
ravisuhag,
rohilsurana and
rsbh
Contributor
Author
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
packages/raystack/components/data-view-beta/components/ordering.tsx (1)
60-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an accessible label to the sort-order toggle.
This icon-only
IconButtonhas no accessible name. Screen-reader users cannot determine the button's purpose or current state.♿ Proposed fix
<IconButton onClick={handleOrderChange} size={4} disabled={columnList.length === 0} + aria-label={ + value.order === SortOrders.ASC + ? 'Switch to descending order' + : 'Switch to ascending order' + } >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/components/ordering.tsx` around lines 60 - 72, The IconButton used as the sort-order toggle in ordering.tsx is icon-only and lacks an accessible name; update the IconButton (the one with onClick={handleOrderChange}, size={4}, disabled={columnList.length === 0}) to include an accessible label that reflects the current state (use aria-label or aria-label plus aria-pressed). Construct the label from value.order and SortOrders (e.g., "Sort ascending" vs "Sort descending") so screen readers know the button's purpose and current state, and keep the existing icons (TextAlignBottomIcon/TextAlignTopIcon) and className intact.packages/raystack/components/data-view-beta/components/filters.tsx (1)
46-50:⚠️ Potential issue | 🟠 MajorAdd an accessible label to the icon-only filter trigger.
Line 48 renders an icon-only
IconButtonwithout an accessible name.Proposed fix
- <IconButton size={4}> + <IconButton size={4} aria-label='Add filter'> <FilterIcon /> </IconButton>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/components/filters.tsx` around lines 46 - 50, The IconButton rendered in the else-if branch when appliedFiltersSet.size > 0 is icon-only and lacks an accessible name; update that IconButton (the one rendering <FilterIcon />) to include an accessible label such as an aria-label (for example aria-label="Open filters" or aria-label={`Filters (${appliedFiltersSet.size})`}) so screen readers can identify the control, ensuring the change is applied to the IconButton in the filters component.packages/raystack/components/data-view-beta/utils/index.tsx (3)
25-29:⚠️ Potential issue | 🟠 MajorReject empty date values before Day.js validity checks.
Both filter sanitization paths call
dayjs(value).isValid()without first rejectingnull/undefined/'', which can incorrectly keep invalid date filters.For Day.js v1.11.x, does dayjs(undefined).isValid() return true, and what pre-check is recommended for null/undefined/empty date filter inputs?Also applies to: 225-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/utils/index.tsx` around lines 25 - 29, The filter currently calls dayjs(data.value).isValid() without rejecting null/undefined/empty values, so update the predicate in the .filter branch that handles FilterType.date to first check the value is not null/undefined/empty (e.g., data.value != null && data.value !== '') before calling dayjs(...).isValid(); apply the same change to the other occurrence (the similar filter at lines referenced in the review) so both uses of dayjs(value).isValid() validate non-empty inputs first.
264-316:⚠️ Potential issue | 🟠 MajorPreserve typed filter values and metadata in query round-trips.
dataViewQueryToInternalcan emitvalue: undefined(when only typed fields are present) and always drops_type/_dataType, making transformations lossy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/utils/index.tsx` around lines 264 - 316, The current mapping in internalFilters (inside the filters.map block, i.e., dataViewQueryToInternal) discards typed value fields and metadata by setting value possibly to undefined and dropping _type/_dataType; fix it by preserving and copying through any of stringValue/numberValue/boolValue into the returned InternalFilter when present (do not replace them when you set transformedFilter), ensure value is set to the original typed field if value is undefined, and include _type and _dataType from filterRest in the returned object; when you alter operator for the 'ilike' wildcard handling keep the existing typed fields and metadata instead of replacing the whole transformedFilter so no typed data or _type/_dataType are lost.
148-168:⚠️ Potential issue | 🟠 MajorMake sort/group change detection order-sensitive.
Current implementations use map/set membership and miss reorder-only changes (e.g., sort priority changes, group order changes).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/utils/index.tsx` around lines 148 - 168, isSortChanged and isGroupChanged are currently order-insensitive (they use generateSortMap/Set) and therefore fail to detect reorder-only changes; update isSortChanged to first check lengths then iterate by index comparing each DataViewSort item's key and order (use the same property names used in your DataViewSort objects) so any change in priority/order is detected, and update isGroupChanged to compare oldGroupBy and newGroupBy element-by-element (index-wise) rather than via a Set so reordered group keys return true; modify the logic inside the isSortChanged and isGroupChanged functions accordingly while keeping their signatures.packages/raystack/components/data-view-beta/utils/filter-operations.tsx (2)
121-150:⚠️ Potential issue | 🟠 MajorNormalize
null/undefinedas empty for select and multiselect filters.Current empty handling only maps
EmptyFilterValue; rows withnull/undefinedwon’t match “empty” consistently forselect.eq/neqandmultiselect.in/notin.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/utils/filter-operations.tsx` around lines 121 - 150, select.eq/select.neq and multiselect.in/multiselect.notin currently only treat the special EmptyFilterValue token as empty, so rows whose value is null/undefined are not matched as empty; update these four functions to normalize both the row value and any filter values: coerce row.getValue(columnId) to '' when it is null or undefined before comparing, treat filterValue.value null/undefined as equivalent to EmptyFilterValue (map to ''), and for multiselect.map the array elements so null/undefined become '' (same as EmptyFilterValue) before includes checks; this ensures select.eq/neq and multiselect.in/notin handle null/undefined consistently as empty.
83-119:⚠️ Potential issue | 🟠 MajorValidate parsed dates before all day comparisons.
dayjs(row.getValue(...))is used directly in every date comparator; invalid/missing values can be treated unexpectedly and produce false matches. Gate comparisons onisValid()for both row and filter dates first.In Day.js, what does dayjs(undefined).isValid() return, and what is the recommended validation pattern before isSame/isBefore/isAfter comparisons?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/utils/filter-operations.tsx` around lines 83 - 119, The date comparator functions (date.eq, neq, lt, lte, gt, gte) call dayjs(...) directly and can yield incorrect matches for invalid/missing inputs; wrap each comparator so you first create two Day.js instances (e.g., rowDate = dayjs(row.getValue(columnId)) and filterDate = dayjs(filterValue.date)), check rowDate.isValid() && filterDate.isValid(), and only then perform the isSame/isBefore/isAfter/isSameOrBefore/isSameOrAfter comparisons; if either date is invalid return false (or the appropriate non-match) to avoid false positives.
🧹 Nitpick comments (2)
packages/raystack/components/data-view-beta/components/ordering.tsx (1)
65-65: 💤 Low valueRemove unnecessary optional chaining on a statically-imported constant.
SortOrdersis a statically imported object and can never benullorundefined, soSortOrders?.ASCis meaningless. It may also mislead readers into thinking the import is nullable.♻️ Proposed fix
- {value.order === SortOrders?.ASC ? ( + {value.order === SortOrders.ASC ? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/components/ordering.tsx` at line 65, Remove the unnecessary optional chaining when comparing value.order to the statically imported SortOrders constant: replace any occurrences of "SortOrders?.ASC" with "SortOrders.ASC" (references include the SortOrders object and its ASC property and the value.order comparison inside the ordering component/render logic). Ensure all other uses in ordering.tsx follow the same pattern so readers don't assume SortOrders can be null/undefined.packages/raystack/components/data-view-beta/components/content.tsx (1)
296-309: 💤 Low valueConsider sourcing empty/zero state from context.
DataViewRootalready exposesisZeroState,isEmptyState, andhasDataon the context, and these values are recomputed locally here. Reading them fromuseDataView()would keep the empty/zero distinction in one place and prevent drift if the root logic evolves (the AI summary explicitly calls out "computed in one place").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/data-view-beta/components/content.tsx` around lines 296 - 309, The component recomputes hasData/isZeroState/isEmptyState locally; instead import the values from the DataViewRoot context via useDataView() and use those to decide stateToShow so the empty/zero logic stays centralized. Replace the local references to hasData, isZeroState, and isEmptyState in the stateToShow calculation (and remove the local computations that use rows/isLoading/hasActiveQuery) and keep using zeroState, emptyState, and DefaultEmptyComponent as fallbacks; ensure you still reference renderedAccessors only where needed and keep hasActiveQuery/tableQuery/defaultSort untouched elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/data-view-beta/components/custom.tsx`:
- Around line 33-36: The effect depends on the array prop fieldsOverride which
will re-trigger on every parent render if passed inline; either require
consumers to provide a stable/memoized array (add a prominent JSDoc on the
fieldsOverride prop in DataView.Custom saying “memoize this with useMemo or keep
stable reference”) or add a ref-based guard inside the useEffect in this file:
keep previousFields in a useRef, perform a deep-equality check (or stable key
compare) between previousFields.current and fieldsOverride and only call
registerFieldsForView(name, fieldsOverride) (and update previousFields.current)
when they differ; reference fieldsOverride, useEffect, registerFieldsForView and
the DataView.Custom fields prop when making the change.
In `@packages/raystack/components/data-view-beta/components/search.tsx`:
- Around line 53-61: The value prop for the Search component can be undefined
(tableQuery?.search) which causes an uncontrolled→controlled input warning;
update the render to pass a stable string (e.g., default to empty string) for
the value used by Search so it is always controlled. Locate the JSX that returns
<Search {...props} onChange={handleSearch} value={tableQuery?.search}
onClear={handleClear} disabled={isDisabled} /> and change the value expression
to use a fallback (nullish or logical fallback) so Search always receives a
string.
In
`@packages/raystack/components/data-view-beta/components/virtualized-content.tsx`:
- Around line 331-346: handleVirtualScroll relies only on onScroll so when
initial content is shorter than the container loadMoreData may never fire;
update the component to run an extra check after layout settles (e.g., in a
useLayoutEffect or after totalHeight/rows change) that reads
scrollContainerRef.current (scrollHeight, clientHeight) and calls loadMoreData
when scrollHeight <= clientHeight + loadMoreOffset and not isLoading and there’s
more data, or alternatively add an IntersectionObserver sentinel at the end of
the list (similar to Content/DataViewList) to trigger loadMoreData; modify
references in handleVirtualScroll, loadMoreData, scrollContainerRef,
loadMoreOffset and updateStickyGroup accordingly so the same load trigger works
both on scroll and on initial render.
In `@packages/raystack/components/data-view-beta/data-view.module.css`:
- Around line 1-20: Remove the extra blank lines inside the CSS rule blocks to
satisfy the declaration-empty-line-before rule: in .toolbar remove the blank
line between the align-self and border-bottom/background declarations so all
declarations are contiguous, and in .display-popover-properties-container ensure
the custom property (--select-width) and subsequent declarations (padding,
border-bottom) are not separated by an empty line; update formatting accordingly
and re-run stylelint to confirm the violations are resolved.
In `@packages/raystack/components/data-view-beta/data-view.tsx`:
- Around line 114-121: initialColumnVisibility is only computed from the root
fields and never updated when a renderer supplies per-view
effectiveFields/fieldsOverride, so columnVisibility state (used by TanStack)
doesn't reflect per-view defaults; update the logic to derive defaults from
effectiveFields and merge them into columnVisibility when effectiveFields
changes while preserving any user-toggled entries: call
getInitialColumnVisibility(effectiveFields) to produce defaultVisibility and in
a useEffect merge defaultVisibility with existing columnVisibility (only set
keys that don't exist in columnVisibility or that were not explicitly toggled),
then setColumnVisibility with the merged map; ensure the same fix is applied to
the other instance mentioned (around the second block using
initialColumnVisibility).
In `@packages/raystack/components/data-view-beta/hooks/useFilters.tsx`:
- Around line 18-24: defaultValue currently falls back to an empty string for
FilterType.multiselect which breaks expected array operations; update the
fallback logic in useFilters.tsx so that when filterType ===
FilterType.multiselect the defaultValue is an empty array (unless
field.defaultFilterValue is provided), and keep existing behavior for
date/select/other types; refer to the defaultValue expression and
FilterType.multiselect to locate and change the conditional.
In `@packages/raystack/components/data-view-beta/utils/filter-operations.tsx`:
- Around line 189-197: The multiselect branch (case FilterType.multiselect)
assumes value is an array and calls value.map(), which will throw if value is
'', null, or undefined; guard the shape by normalizing value to an array before
mapping (e.g., treat non-array or falsy values as []), then map each item
replacing EmptyFilterValue with '' and join to produce stringValue; update the
logic in the FilterType.multiselect return to use this normalized array so
stringValue never tries to call .map on a non-array.
---
Duplicate comments:
In `@packages/raystack/components/data-view-beta/components/filters.tsx`:
- Around line 46-50: The IconButton rendered in the else-if branch when
appliedFiltersSet.size > 0 is icon-only and lacks an accessible name; update
that IconButton (the one rendering <FilterIcon />) to include an accessible
label such as an aria-label (for example aria-label="Open filters" or
aria-label={`Filters (${appliedFiltersSet.size})`}) so screen readers can
identify the control, ensuring the change is applied to the IconButton in the
filters component.
In `@packages/raystack/components/data-view-beta/components/ordering.tsx`:
- Around line 60-72: The IconButton used as the sort-order toggle in
ordering.tsx is icon-only and lacks an accessible name; update the IconButton
(the one with onClick={handleOrderChange}, size={4}, disabled={columnList.length
=== 0}) to include an accessible label that reflects the current state (use
aria-label or aria-label plus aria-pressed). Construct the label from
value.order and SortOrders (e.g., "Sort ascending" vs "Sort descending") so
screen readers know the button's purpose and current state, and keep the
existing icons (TextAlignBottomIcon/TextAlignTopIcon) and className intact.
In `@packages/raystack/components/data-view-beta/utils/filter-operations.tsx`:
- Around line 121-150: select.eq/select.neq and multiselect.in/multiselect.notin
currently only treat the special EmptyFilterValue token as empty, so rows whose
value is null/undefined are not matched as empty; update these four functions to
normalize both the row value and any filter values: coerce
row.getValue(columnId) to '' when it is null or undefined before comparing,
treat filterValue.value null/undefined as equivalent to EmptyFilterValue (map to
''), and for multiselect.map the array elements so null/undefined become ''
(same as EmptyFilterValue) before includes checks; this ensures select.eq/neq
and multiselect.in/notin handle null/undefined consistently as empty.
- Around line 83-119: The date comparator functions (date.eq, neq, lt, lte, gt,
gte) call dayjs(...) directly and can yield incorrect matches for
invalid/missing inputs; wrap each comparator so you first create two Day.js
instances (e.g., rowDate = dayjs(row.getValue(columnId)) and filterDate =
dayjs(filterValue.date)), check rowDate.isValid() && filterDate.isValid(), and
only then perform the isSame/isBefore/isAfter/isSameOrBefore/isSameOrAfter
comparisons; if either date is invalid return false (or the appropriate
non-match) to avoid false positives.
In `@packages/raystack/components/data-view-beta/utils/index.tsx`:
- Around line 25-29: The filter currently calls dayjs(data.value).isValid()
without rejecting null/undefined/empty values, so update the predicate in the
.filter branch that handles FilterType.date to first check the value is not
null/undefined/empty (e.g., data.value != null && data.value !== '') before
calling dayjs(...).isValid(); apply the same change to the other occurrence (the
similar filter at lines referenced in the review) so both uses of
dayjs(value).isValid() validate non-empty inputs first.
- Around line 264-316: The current mapping in internalFilters (inside the
filters.map block, i.e., dataViewQueryToInternal) discards typed value fields
and metadata by setting value possibly to undefined and dropping
_type/_dataType; fix it by preserving and copying through any of
stringValue/numberValue/boolValue into the returned InternalFilter when present
(do not replace them when you set transformedFilter), ensure value is set to the
original typed field if value is undefined, and include _type and _dataType from
filterRest in the returned object; when you alter operator for the 'ilike'
wildcard handling keep the existing typed fields and metadata instead of
replacing the whole transformedFilter so no typed data or _type/_dataType are
lost.
- Around line 148-168: isSortChanged and isGroupChanged are currently
order-insensitive (they use generateSortMap/Set) and therefore fail to detect
reorder-only changes; update isSortChanged to first check lengths then iterate
by index comparing each DataViewSort item's key and order (use the same property
names used in your DataViewSort objects) so any change in priority/order is
detected, and update isGroupChanged to compare oldGroupBy and newGroupBy
element-by-element (index-wise) rather than via a Set so reordered group keys
return true; modify the logic inside the isSortChanged and isGroupChanged
functions accordingly while keeping their signatures.
---
Nitpick comments:
In `@packages/raystack/components/data-view-beta/components/content.tsx`:
- Around line 296-309: The component recomputes hasData/isZeroState/isEmptyState
locally; instead import the values from the DataViewRoot context via
useDataView() and use those to decide stateToShow so the empty/zero logic stays
centralized. Replace the local references to hasData, isZeroState, and
isEmptyState in the stateToShow calculation (and remove the local computations
that use rows/isLoading/hasActiveQuery) and keep using zeroState, emptyState,
and DefaultEmptyComponent as fallbacks; ensure you still reference
renderedAccessors only where needed and keep
hasActiveQuery/tableQuery/defaultSort untouched elsewhere.
In `@packages/raystack/components/data-view-beta/components/ordering.tsx`:
- Line 65: Remove the unnecessary optional chaining when comparing value.order
to the statically imported SortOrders constant: replace any occurrences of
"SortOrders?.ASC" with "SortOrders.ASC" (references include the SortOrders
object and its ASC property and the value.order comparison inside the ordering
component/render logic). Ensure all other uses in ordering.tsx follow the same
pattern so readers don't assume SortOrders can be null/undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 499b1de3-df5e-45e4-9d14-4339d357bc43
📒 Files selected for processing (29)
apps/www/src/app/examples/dataview/page.tsxdocs/rfcs/002-unified-dataview-component.mdpackages/raystack/components/data-view-beta/components/content.tsxpackages/raystack/components/data-view-beta/components/custom.tsxpackages/raystack/components/data-view-beta/components/display-access.tsxpackages/raystack/components/data-view-beta/components/display-properties.tsxpackages/raystack/components/data-view-beta/components/display-settings.tsxpackages/raystack/components/data-view-beta/components/empty-state.tsxpackages/raystack/components/data-view-beta/components/filters.tsxpackages/raystack/components/data-view-beta/components/grouping.tsxpackages/raystack/components/data-view-beta/components/list.tsxpackages/raystack/components/data-view-beta/components/ordering.tsxpackages/raystack/components/data-view-beta/components/renderer.tsxpackages/raystack/components/data-view-beta/components/search.tsxpackages/raystack/components/data-view-beta/components/table.tsxpackages/raystack/components/data-view-beta/components/toolbar.tsxpackages/raystack/components/data-view-beta/components/view-switcher.tsxpackages/raystack/components/data-view-beta/components/virtualized-content.tsxpackages/raystack/components/data-view-beta/components/zero-state.tsxpackages/raystack/components/data-view-beta/context.tsxpackages/raystack/components/data-view-beta/data-view.module.csspackages/raystack/components/data-view-beta/data-view.tsxpackages/raystack/components/data-view-beta/data-view.types.tsxpackages/raystack/components/data-view-beta/hooks/useDataView.tsxpackages/raystack/components/data-view-beta/hooks/useFilters.tsxpackages/raystack/components/data-view-beta/index.tspackages/raystack/components/data-view-beta/utils/filter-operations.tsxpackages/raystack/components/data-view-beta/utils/index.tsxpackages/raystack/index.tsx
✅ Files skipped from review due to trivial changes (4)
- packages/raystack/components/data-view-beta/components/table.tsx
- packages/raystack/components/data-view-beta/components/display-properties.tsx
- packages/raystack/components/data-view-beta/index.ts
- packages/raystack/components/data-view-beta/data-view.types.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/raystack/index.tsx
- apps/www/src/app/examples/dataview/page.tsx
Comment on lines
+33
to
+36
| useEffect(() => { | ||
| if (!name || !fieldsOverride) return; | ||
| return registerFieldsForView(name, fieldsOverride); | ||
| }, [name, fieldsOverride, registerFieldsForView]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether registerFieldsForView is wrapped in useCallback in the provider
rg -n "registerFieldsForView" --type=tsx --type=ts -A 3 -B 3Repository: raystack/apsara
Length of output: 87
🏁 Script executed:
# Search for registerFieldsForView with corrected flags
rg -n "registerFieldsForView" -A 3 -B 3 --type tsRepository: raystack/apsara
Length of output: 5403
🏁 Script executed:
# Also search more broadly without type restriction to find all occurrences
rg -n "registerFieldsForView" -A 3 -B 3Repository: raystack/apsara
Length of output: 5403
🏁 Script executed:
# Locate the DataViewCustom component file mentioned in the review
find . -name "custom.tsx" -path "*/data-view-beta/*"Repository: raystack/apsara
Length of output: 127
🏁 Script executed:
cat -n packages/raystack/components/data-view-beta/data-view.tsx | sed -n '85,105p'Repository: raystack/apsara
Length of output: 831
🏁 Script executed:
cat -n packages/raystack/components/data-view-beta/data-view.tsx | sed -n '91,110p'Repository: raystack/apsara
Length of output: 791
🏁 Script executed:
cat -n packages/raystack/components/data-view-beta/components/custom.tsx | head -50Repository: raystack/apsara
Length of output: 1913
fieldsOverride array reference instability will cause the effect to re-fire on every parent render.
fieldsOverride is DataViewField<TData>[]. Arrays are compared by reference in JavaScript, so passing an inline array literal — the natural usage pattern:
<DataView.Custom name="cards" fields={[{ accessorKey: 'name', ... }]}>…creates a new array on every parent render. React's useEffect sees a changed dependency and re-runs, calling registerFieldsForView on every render cycle. This causes unnecessary field registration and cleanup cycles.
The fix is to require consumers to memoize fieldsOverride. Add a prominent JSDoc warning:
export interface DataViewCustomProps<TData> {
/** Multi-view name. When set, the renderer gates itself on the active view. */
name?: string;
- /** Optional view-scoped field override. Full replacement of root `fields` for this view's active session. */
+ /**
+ * Optional view-scoped field override. Full replacement of root `fields` for this view's active session.
+ * **Must be referentially stable** (e.g. defined outside the render function or wrapped in `useMemo`)
+ * to prevent the registration effect from re-firing on every render.
+ */
fields?: DataViewField<TData>[];Alternatively, guard the effect with a ref-based deep equality check to skip redundant registrations when field definitions haven't semantically changed:
♻️ Alternative fix using a ref guard
+import { ReactNode, useEffect, useRef } from 'react';
+import { isEqual } from 'some-deep-equal-utility'; // or JSON.stringify comparison
export function DataViewCustom<TData>({
name,
fields: fieldsOverride,
children
}: DataViewCustomProps<TData>) {
const ctx = useDataView<TData>();
const { activeView, registerFieldsForView } = ctx;
+ const prevFieldsRef = useRef<DataViewField<TData>[] | undefined>(undefined);
useEffect(() => {
if (!name || !fieldsOverride) return;
+ if (isEqual(prevFieldsRef.current, fieldsOverride)) return;
+ prevFieldsRef.current = fieldsOverride;
return registerFieldsForView(name, fieldsOverride);
}, [name, fieldsOverride, registerFieldsForView]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/components/custom.tsx` around
lines 33 - 36, The effect depends on the array prop fieldsOverride which will
re-trigger on every parent render if passed inline; either require consumers to
provide a stable/memoized array (add a prominent JSDoc on the fieldsOverride
prop in DataView.Custom saying “memoize this with useMemo or keep stable
reference”) or add a ref-based guard inside the useEffect in this file: keep
previousFields in a useRef, perform a deep-equality check (or stable key
compare) between previousFields.current and fieldsOverride and only call
registerFieldsForView(name, fieldsOverride) (and update previousFields.current)
when they differ; reference fieldsOverride, useEffect, registerFieldsForView and
the DataView.Custom fields prop when making the change.
Comment on lines
+53
to
+61
| return ( | ||
| <Search | ||
| {...props} | ||
| onChange={handleSearch} | ||
| value={tableQuery?.search} | ||
| onClear={handleClear} | ||
| disabled={isDisabled} | ||
| /> | ||
| ); |
Contributor
There was a problem hiding this comment.
value prop may be undefined, causing an uncontrolled→controlled React warning.
tableQuery?.search resolves to undefined if either tableQuery is nullish or search is not initialized on the query object. Passing undefined to a controlled <input> (via Search) makes it uncontrolled; the first keystroke would switch it to controlled, producing a React warning and potentially losing the cursor position.
🛡️ Proposed fix
- value={tableQuery?.search}
+ value={tableQuery?.search ?? ''}📝 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.
Suggested change
| return ( | |
| <Search | |
| {...props} | |
| onChange={handleSearch} | |
| value={tableQuery?.search} | |
| onClear={handleClear} | |
| disabled={isDisabled} | |
| /> | |
| ); | |
| return ( | |
| <Search | |
| {...props} | |
| onChange={handleSearch} | |
| value={tableQuery?.search ?? ''} | |
| onClear={handleClear} | |
| disabled={isDisabled} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/components/search.tsx` around
lines 53 - 61, The value prop for the Search component can be undefined
(tableQuery?.search) which causes an uncontrolled→controlled input warning;
update the render to pass a stable string (e.g., default to empty string) for
the value used by Search so it is always controlled. Locate the JSX that returns
<Search {...props} onChange={handleSearch} value={tableQuery?.search}
onClear={handleClear} disabled={isDisabled} /> and change the value expression
to use a fallback (nullish or logical fallback) so Search always receives a
string.
Comment on lines
+331
to
+346
| const handleVirtualScroll = useCallback(() => { | ||
| const el = scrollContainerRef.current; | ||
| if (!el) return; | ||
| if (stickyGroupHeader) updateStickyGroup(); | ||
| if (isLoading) return; | ||
| const { scrollTop, scrollHeight, clientHeight } = el; | ||
| if (scrollHeight - scrollTop - clientHeight < loadMoreOffset!) { | ||
| loadMoreData(); | ||
| } | ||
| }, [ | ||
| stickyGroupHeader, | ||
| isLoading, | ||
| loadMoreData, | ||
| loadMoreOffset, | ||
| updateStickyGroup | ||
| ]); |
Contributor
There was a problem hiding this comment.
loadMoreData may never trigger on short content in server mode.
Infinite loading here is driven exclusively by onScroll. If the initial page of rows fits within the scroll container (or the user never scrolls), handleVirtualScroll won't run and loadMoreData won't be called, so the next page is never fetched. Consider triggering an extra check after rows/totalHeight settle — e.g., after the layout effect — when scrollHeight <= clientHeight + loadMoreOffset and there's more data on the server, or fall back to an IntersectionObserver on a sentinel like Content/DataViewList does.
Also applies to: 377-382
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/raystack/components/data-view-beta/components/virtualized-content.tsx`
around lines 331 - 346, handleVirtualScroll relies only on onScroll so when
initial content is shorter than the container loadMoreData may never fire;
update the component to run an extra check after layout settles (e.g., in a
useLayoutEffect or after totalHeight/rows change) that reads
scrollContainerRef.current (scrollHeight, clientHeight) and calls loadMoreData
when scrollHeight <= clientHeight + loadMoreOffset and not isLoading and there’s
more data, or alternatively add an IntersectionObserver sentinel at the end of
the list (similar to Content/DataViewList) to trigger loadMoreData; modify
references in handleVirtualScroll, loadMoreData, scrollContainerRef,
loadMoreOffset and updateStickyGroup accordingly so the same load trigger works
both on scroll and on initial render.
Comment on lines
+1
to
+20
| .toolbar { | ||
| padding: var(--rs-space-3) 0; | ||
| align-self: stretch; | ||
|
|
||
| border-bottom: 0.5px solid var(--rs-color-border-base-primary); | ||
| background: var(--rs-color-background-base-primary); | ||
| } | ||
|
|
||
| .display-popover-content { | ||
| padding: 0px; | ||
| /* Todo: var does not exist for 300px */ | ||
| min-width: 300px; | ||
| } | ||
|
|
||
| .display-popover-properties-container { | ||
| /* Todo: var does not exist for 160px */ | ||
| --select-width: 160px; | ||
| padding: var(--rs-space-5); | ||
| border-bottom: 1px solid var(--rs-color-border-base-primary); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix stylelint declaration-empty-line-before violations.
Static analysis flags formatting errors around Line 5 and Line 18; this can fail lint checks.
Proposed fix
.toolbar {
padding: var(--rs-space-3) 0;
align-self: stretch;
-
border-bottom: 0.5px solid var(--rs-color-border-base-primary);
background: var(--rs-color-background-base-primary);
}
@@
.display-popover-properties-container {
/* Todo: var does not exist for 160px */
--select-width: 160px;
+
padding: var(--rs-space-5);
border-bottom: 1px solid var(--rs-color-border-base-primary);
}📝 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.
Suggested change
| .toolbar { | |
| padding: var(--rs-space-3) 0; | |
| align-self: stretch; | |
| border-bottom: 0.5px solid var(--rs-color-border-base-primary); | |
| background: var(--rs-color-background-base-primary); | |
| } | |
| .display-popover-content { | |
| padding: 0px; | |
| /* Todo: var does not exist for 300px */ | |
| min-width: 300px; | |
| } | |
| .display-popover-properties-container { | |
| /* Todo: var does not exist for 160px */ | |
| --select-width: 160px; | |
| padding: var(--rs-space-5); | |
| border-bottom: 1px solid var(--rs-color-border-base-primary); | |
| } | |
| .toolbar { | |
| padding: var(--rs-space-3) 0; | |
| align-self: stretch; | |
| border-bottom: 0.5px solid var(--rs-color-border-base-primary); | |
| background: var(--rs-color-background-base-primary); | |
| } | |
| .display-popover-content { | |
| padding: 0px; | |
| /* Todo: var does not exist for 300px */ | |
| min-width: 300px; | |
| } | |
| .display-popover-properties-container { | |
| /* Todo: var does not exist for 160px */ | |
| --select-width: 160px; | |
| padding: var(--rs-space-5); | |
| border-bottom: 1px solid var(--rs-color-border-base-primary); | |
| } |
🧰 Tools
🪛 Stylelint (17.10.0)
[error] 5-5: Expected no empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 18-18: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/data-view.module.css` around
lines 1 - 20, Remove the extra blank lines inside the CSS rule blocks to satisfy
the declaration-empty-line-before rule: in .toolbar remove the blank line
between the align-self and border-bottom/background declarations so all
declarations are contiguous, and in .display-popover-properties-container ensure
the custom property (--select-width) and subsequent declarations (padding,
border-bottom) are not separated by an empty line; update formatting accordingly
and re-run stylelint to confirm the violations are resolved.
Comment on lines
+114
to
+121
| const initialColumnVisibility = useMemo( | ||
| () => getInitialColumnVisibility(fields), | ||
| [fields] | ||
| ); | ||
|
|
||
| const [columnVisibility, setColumnVisibility] = useState<VisibilityState>( | ||
| initialColumnVisibility | ||
| ); |
Contributor
There was a problem hiding this comment.
Per-view field overrides don't update column visibility defaults.
initialColumnVisibility is computed once from the root fields and stored in columnVisibility state. When a renderer registers a per-view fieldsOverride (changing effectiveFields/columnDefs), the visibility map is never re-derived, so:
- Override fields with
displayed: falsewill be rendered as visible (no entry → TanStack defaults to visible). - Override fields that hide a column the root has visible won't take effect.
Recompute the visibility map when effectiveFields changes (preserving any user-toggled values), or at minimum derive it from effectiveFields so per-view defaults are honored.
Direction
- const initialColumnVisibility = useMemo(
- () => getInitialColumnVisibility(fields),
- [fields]
- );
+ const initialColumnVisibility = useMemo(
+ () => getInitialColumnVisibility(effectiveFields),
+ [effectiveFields]
+ );
@@
- const [columnVisibility, setColumnVisibility] = useState<VisibilityState>(
- initialColumnVisibility
- );
+ const [columnVisibility, setColumnVisibility] = useState<VisibilityState>(
+ initialColumnVisibility
+ );
+
+ // Merge defaults from the active view's fields without clobbering user toggles.
+ useEffect(() => {
+ setColumnVisibility(prev => ({ ...initialColumnVisibility, ...prev }));
+ }, [initialColumnVisibility]);Also applies to: 184-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/data-view.tsx` around lines 114 -
121, initialColumnVisibility is only computed from the root fields and never
updated when a renderer supplies per-view effectiveFields/fieldsOverride, so
columnVisibility state (used by TanStack) doesn't reflect per-view defaults;
update the logic to derive defaults from effectiveFields and merge them into
columnVisibility when effectiveFields changes while preserving any user-toggled
entries: call getInitialColumnVisibility(effectiveFields) to produce
defaultVisibility and in a useEffect merge defaultVisibility with existing
columnVisibility (only set keys that don't exist in columnVisibility or that
were not explicitly toggled), then setColumnVisibility with the merged map;
ensure the same fix is applied to the other instance mentioned (around the
second block using initialColumnVisibility).
Comment on lines
+18
to
+24
| const defaultValue = | ||
| field.defaultFilterValue ?? | ||
| (filterType === FilterType.date | ||
| ? new Date() | ||
| : filterType === FilterType.select | ||
| ? options[0]?.value | ||
| : ''); |
Contributor
There was a problem hiding this comment.
Use an array default for FilterType.multiselect.
Line 24 currently defaults multiselect filters to ''; this breaks expected value shape and can crash later when array operations run.
Proposed fix
const defaultValue =
field.defaultFilterValue ??
(filterType === FilterType.date
? new Date()
: filterType === FilterType.select
? options[0]?.value
+ : filterType === FilterType.multiselect
+ ? []
: '');📝 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.
Suggested change
| const defaultValue = | |
| field.defaultFilterValue ?? | |
| (filterType === FilterType.date | |
| ? new Date() | |
| : filterType === FilterType.select | |
| ? options[0]?.value | |
| : ''); | |
| const defaultValue = | |
| field.defaultFilterValue ?? | |
| (filterType === FilterType.date | |
| ? new Date() | |
| : filterType === FilterType.select | |
| ? options[0]?.value | |
| : filterType === FilterType.multiselect | |
| ? [] | |
| : ''); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/hooks/useFilters.tsx` around
lines 18 - 24, defaultValue currently falls back to an empty string for
FilterType.multiselect which breaks expected array operations; update the
fallback logic in useFilters.tsx so that when filterType ===
FilterType.multiselect the defaultValue is an empty array (unless
field.defaultFilterValue is provided), and keep existing behavior for
date/select/other types; refer to the defaultValue expression and
FilterType.multiselect to locate and change the conditional.
Comment on lines
+189
to
+197
| case FilterType.multiselect: | ||
| return { | ||
| value, | ||
| stringValue: value | ||
| .map((value: any) => | ||
| value === EmptyFilterValue ? '' : String(value) | ||
| ) | ||
| .join() | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard multiselect value shape before calling .map().
FilterType.multiselect assumes value is an array. If it is ''/null/undefined, this throws at runtime.
Proposed fix
case FilterType.multiselect:
+ if (!Array.isArray(value)) {
+ return { value, stringValue: '' };
+ }
return {
value,
stringValue: value
.map((value: any) =>
value === EmptyFilterValue ? '' : String(value)
)
.join()
};📝 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.
Suggested change
| case FilterType.multiselect: | |
| return { | |
| value, | |
| stringValue: value | |
| .map((value: any) => | |
| value === EmptyFilterValue ? '' : String(value) | |
| ) | |
| .join() | |
| }; | |
| case FilterType.multiselect: | |
| if (!Array.isArray(value)) { | |
| return { value, stringValue: '' }; | |
| } | |
| return { | |
| value, | |
| stringValue: value | |
| .map((value: any) => | |
| value === EmptyFilterValue ? '' : String(value) | |
| ) | |
| .join() | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-view-beta/utils/filter-operations.tsx`
around lines 189 - 197, The multiselect branch (case FilterType.multiselect)
assumes value is an array and calls value.map(), which will throw if value is
'', null, or undefined; guard the shape by normalizing value to an array before
mapping (e.g., treat non-array or falsy values as []), then map each item
replacing EmptyFilterValue with '' and join to produce stringValue; update the
logic in the FilterType.multiselect return to use this normalized array so
stringValue never tries to call .map on a non-array.
rohilsurana
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Preview RFC
Preview Example