Conversation
…etting filters on data change
…ter state in DataTable filtering
|
Could you add screenshots or a short video showing your testing results? |
|
https://github.com/user-attachments/assets/cb4ba639-7e55-4033-9be9-47ab2e431d27 |
…taTable Filters now use declared intent instead of heuristic type detection: - >, <, >=, <= are always numeric (no string fallback) - "date" prefix required for date comparisons (no auto-detection) - = / != auto-detect numeric vs string exact match - Bare text remains case-insensitive substring match
There was a problem hiding this comment.
Pull request overview
Adds inline per-column filtering to the frontend DataTable so users can filter table rows directly in the UI (used in Sidekick data preview and code block table outputs).
Changes:
- Introduces a
FilterRowUI rendered under column headers whenenableFilteringis enabled. - Adds
filterUtils.tsfor parsing filter expressions and matching rows (substring, comparison ops, date prefix, blank/not blank). - Integrates filtering into
DataTablewith debounced inputs, original row index preservation, and filtered row count reporting toTableOutput.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| mage_ai/frontend/components/Sidekick/index.tsx | Enables filtering in the Sidekick data preview table. |
| mage_ai/frontend/components/DataTable/index.tsx | Implements filtering state, debouncing, empty-filter UI, original index mapping, and the onFilteredRowCount callback. |
| mage_ai/frontend/components/DataTable/filterUtils.ts | New filter parsing/matching logic used by DataTable. |
| mage_ai/frontend/components/DataTable/FilterRow.tsx | New component rendering per-column filter inputs beneath headers. |
| mage_ai/frontend/components/CodeBlock/CodeOutput/TableOutput.tsx | Enables filtering in table output and shows “Showing X of Y rows” when filtered. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [prevData, setPrevData] = useState(data); | ||
| if (data !== prevData) { | ||
| setPrevData(data); | ||
| setFilters({}); | ||
| setDebouncedFilters({}); | ||
| cancelDebounce(); | ||
| } | ||
|
|
There was a problem hiding this comment.
State is being updated during render (if (data !== prevData) { setPrevData(...); setFilters(...); ... }). This can trigger React warnings and extra renders (and cancelDebounce() is also a side effect during render). Move this reset logic into a useEffect (or use a useRef to track previous data) keyed on data so updates happen after render.
| const [prevData, setPrevData] = useState(data); | |
| if (data !== prevData) { | |
| setPrevData(data); | |
| setFilters({}); | |
| setDebouncedFilters({}); | |
| cancelDebounce(); | |
| } | |
| const prevDataRef = useRef(data); | |
| useEffect(() => { | |
| if (data !== prevDataRef.current) { | |
| prevDataRef.current = data; | |
| setFilters({}); | |
| setDebouncedFilters({}); | |
| cancelDebounce(); | |
| } | |
| }, [data, cancelDebounce]); |
There was a problem hiding this comment.
This is a supported React pattern documented here: https://react.dev/reference/react/useState#storing-information-from-previous-renders
React considers it a last resort — they recommend first considering computed values, key-based resets, or event handlers. But for this case:
- Filters are user-entered state, not derivable from data
- A key reset would blow away all component state (scroll position, etc.)
- data arrives as a prop, not from a user event in this component
So the during-render pattern is the appropriate choice here. Critically, the docs say: "However, it's better than updating state in an effect."
Moving this to a useEffect would cause the component to render one frame with new data but stale filters (a visible flash of incorrect state), then fire the effect and re-render with cleared filters. The during-render approach avoids that by clearing filters before the browser paints.
cancelDebounce() is just clearing a timeout — a trivial synchronous operation, so it's fine here.
| indexColumnValue = indexColumnValue[idx]; | ||
| } | ||
| } else if (isFiltering) { | ||
| indexColumnValue = String(displayIndex); |
There was a problem hiding this comment.
When filtering and shouldUseIndexProp is false, the index column uses String(displayIndex) which is always zero-based. This bypasses the disableZeroIndexRowNumber behavior implemented in buildIndexColumns (and can change displayed row numbers when filtering is enabled). Consider reusing the same index calculation as the index column accessor (e.g., add the +1 offset when disableZeroIndexRowNumber is true) rather than hardcoding String(displayIndex).
| indexColumnValue = String(displayIndex); | |
| const rowNumber = disableZeroIndexRowNumber ? displayIndex + 1 : displayIndex; | |
| indexColumnValue = String(rowNumber); |
| {indexColumnWidths.map((w, idx) => ( | ||
| <div | ||
| className="td td-index-column" | ||
| key={`filter-index-${idx}`} | ||
| style={{ | ||
| left: 0, | ||
| minWidth: w, | ||
| position: 'sticky', | ||
| width: w, | ||
| }} |
There was a problem hiding this comment.
All sticky index placeholders in the filter row are positioned with left: 0. If there are multiple index columns (indexColumnWidths.length > 1), these sticky cells will overlap each other. Compute left as the cumulative sum of previous index column widths (or reuse the same sticky positioning approach as the header/body cells) so each sticky column is offset correctly.
There was a problem hiding this comment.
this is a pre-existing pattern across the entire DataTable — the header cells (line 784), body cells (line 575), and this filter row all hardcode left: 0 for every index column. Fixing it only here would create an inconsistency with the header and body rows. I'd prefer to address this as a separate follow-up that fixes all three locations together. In practice, numberOfIndexes is typically 1 (just the row number), so the overlap doesn't manifest in normal usage.
| const { data, resource_usage: resourceUsage, sample_data: sampleData } = output; | ||
| const [filteredRowCount, setFilteredRowCount] = useState<number | null>(null); |
There was a problem hiding this comment.
filteredRowCount state can persist across output changes and briefly show a stale "Showing X of Y rows" until the new DataTable calls onFilteredRowCount. Reset filteredRowCount to null in an effect when the underlying table content changes (e.g., when order, uuid, or output changes) to avoid flashing incorrect counts.
| const cellNumeric = parseFloat(displayValue); | ||
| if (isNaN(cellNumeric)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Numeric comparisons currently parse the cell value with parseFloat(displayValue). This will treat strings like "100abc" as numeric 100, which can produce incorrect matches. Consider validating the cell value with the same strict numeric regex used for filter parsing (or otherwise requiring a fully numeric string) before comparing.
| const cellNumeric = parseFloat(displayValue); | |
| if (isNaN(cellNumeric)) { | |
| return false; | |
| } | |
| const normalizedDisplay = displayValue.trim(); | |
| if (!STRICT_NUMBER_REGEX.test(normalizedDisplay)) { | |
| return false; | |
| } | |
| const cellNumeric = parseFloat(normalizedDisplay); |
Description
Add inline column filtering to the DataTable component, as requested in #4143. Users can filter rows directly from text inputs rendered below the column headers.
Key changes:
shows a "No rows match" message when the filtered result is empty
How Has This Been Tested?
Checklist
cc:
@wangxiaoyou1993