-
Notifications
You must be signed in to change notification settings - Fork 2
Data Table PRO MAX 🔥 #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on October 26
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
if (range.to) result.to = range.to | ||
props.onDateRangeChange && props.onDateRangeChange(result) | ||
} | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stale Closures in Date Picker Callbacks
The formatDate
and onSelect
useCallback
hooks in both DatePicker
and DateRangePicker
components omit relevant props
from their dependency arrays. This can lead to stale closures, causing these callbacks to use outdated prop values if the formatDate
, onDateChange
, or onDateRangeChange
props change.
Additional Locations (2)
pageIndex: newPagination.pageIndex + 1, | ||
pageSize: newPagination.pageSize, | ||
}); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Pagination Indexing Error
The onPaginationChange
callback incorrectly increments the pageIndex
by one before passing it to the external handler. Since the component's pageIndex
prop and the external callback already expect 1-based indexing, this results in a double conversion and an off-by-one error, causing incorrect page numbers to be reported.
const onSelect = useCallback((value: Date | undefined) => { | ||
setOpen(false); | ||
props.onDateChange && props.onDateChange(value); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (range.to) result.to = range.to | ||
props.onDateRangeChange && props.onDateRangeChange(result) | ||
} | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pageCount={pageCount} | ||
currentPage={pageIndex} | ||
canPrevious={table.getCanPreviousPage()} | ||
canNext={pageCount === undefined ? data.length >= pageSize : table.getCanNextPage()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Pagination Logic Fails When Page Count Is Undefined
The canNext
calculation for the pagination component incorrectly uses data.length >= pageSize
when pageCount
is undefined. This logic only checks if the current page is full, potentially enabling the "next" button even on the last page of data.
type="text" | ||
placeholder={filter.placeholder || columnId} | ||
value={filter.value || ""} | ||
onChange={onValueChange} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Filter Components Pass Incorrect Event Data
The TextFilter
and NumberFilter
components pass the raw onChange
event to onValueChange
, but the callback expects just the input's string value. This prevents the filter logic from receiving the correct data.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run linter and formatter. And rebase your branch
left: isPinned === "left" ? `${column.getStart("left")}px` : undefined, | ||
right: isPinned === "right" ? `${column.getAfter("right")}px` : undefined, | ||
position: isPinned ? "sticky" : "relative", | ||
zIndex: isPinned ? 1 : 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need z-index?
[...Array(3)].map((_, index) => ( | ||
<tr key={`skeleton-${index}`} className={cn(styles.dataRow, showZebraStripes && index % 2 === (showFilters ? 0 : 1) && styles.dataRowZebra)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove iteration here
isLoading ? ( | ||
// Show 3 skeleton rows when loading with no data | ||
[...Array(3)].map((_, index) => ( | ||
<tr key={`skeleton-${index}`} className={cn(styles.dataRow, showZebraStripes && index % 2 === (showFilters ? 0 : 1) && styles.dataRowZebra)}> | ||
{table.getVisibleFlatColumns().map((column) => ( | ||
<TableCell | ||
key={`skeleton-${index}-${column.id}`} | ||
column={column} | ||
draggedColumn={draggedColumn} | ||
dropTarget={dropTarget} | ||
> | ||
<Skeleton className="h-5 w-full" /> | ||
</TableCell> | ||
))} | ||
</tr> | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too large conditional
type TableRowProps<T> = { | ||
row: Row<T> | ||
isLoading: boolean | ||
onClick: MouseEventHandler<HTMLTableRowElement> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not onClick from react?
className={cn( | ||
styles.dataRow, | ||
showZebraStripes && | ||
rowIndex % 2 === (showFilters ? 0 : 1) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite hard to read. Why does color depend on showFilters? How about moving this to closure:
const zebraRowStyle = (index) => {
const effectiveIndex = showFilters ? rowIndex : rowIndex + 1;
if (effectiveIndex %2 === 0) {
return styles.dataRowZebra
} else {
return ""
}
}
setDropTarget, | ||
setDraggedColumn, | ||
setColumnOrder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider event-based like onDrop
, onColumDrag
, etc.
const filterInputClasses = "h-full text-sm border-0 bg-transparent focus-visible:ring-0 placeholder:text-[#CCCED3] pl-8 focus:outline-none"; | ||
const filterIconClasses = "absolute left-0 top-1/2 -translate-y-1/2 pointer-events-none"; | ||
|
||
export const TextFilter: React.FC<TextFilterProps> = ({ columnId, onValueChange, filter }: TextFilterProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to nest filter here?
|
||
return ( | ||
<Select | ||
key={`${columnId}-${selectValue || 'empty'}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty is a valid value, so it will break if there is an empty
inside the list.
@@ -1,4 +1,3 @@ | |||
"use client"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
Dropdown: ({ className, ...props }) => { | ||
return ( | ||
<Select | ||
defaultValue={props.value?.toString() || ''} | ||
onValueChange={(val) => props.onChange?.({ target: { value: val } } as any)} | ||
> | ||
<SelectTrigger className="relative w-full" size={"regular"}> | ||
<SelectValue /> | ||
</SelectTrigger> | ||
<SelectContent className="max-h-60 overflow-auto" side="bottom" sideOffset={4}> | ||
{props.options?.map((opt) => ( | ||
<SelectItem key={opt.value} value={opt.value.toString()}> | ||
{opt.label} | ||
</SelectItem> | ||
))} | ||
</SelectContent> | ||
</Select> | ||
) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to data table. Move to separate commit.
New Components:
Fixed Components:
Dropdown
for date select