feat(frontend): implement advanced data table component - #276
Conversation
📝 WalkthroughWalkthroughAdds a reusable MUI ChangesDataTable adoption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuditLogDashboard
participant DataTable
participant TableFilter
participant Storage
AuditLogDashboard->>DataTable: provide logs and auditColumns
TableFilter->>DataTable: update column filter
DataTable->>Storage: persist filters, sorting, and layout
DataTable->>AuditLogDashboard: render filtered, sorted, paginated rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
frontend/src/components/__tests__/TableSortLabel.test.tsx (1)
34-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onRowsPerPageChangeis wired up but never asserted.Consider adding a case that opens the page-size
Selectand asserts the callback fires with the chosen value; the page-size path is currently untested.🤖 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 `@frontend/src/components/__tests__/TableSortLabel.test.tsx` around lines 34 - 57, Extend the TablePagination test in the “renders range and navigates pages” case to open the page-size Select, choose a different option, and assert onRowsPerPageChange receives the selected value. Keep the existing range and page-navigation assertions unchanged.frontend/src/components/DataTable.tsx (3)
407-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVirtualization is effectively unreachable with the default props.
useVirtualis gated onpagedData.length >= virtualizationThreshold(200), but pagination defaults to on withpageSize25, so a page never reaches the threshold. Consumers must setenablePagination={false}or a ≥200 page size to get virtualization. If that's intentional, worth a doc comment; otherwise gate onsortedData.lengthand disable pagination when virtualizing.🤖 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 `@frontend/src/components/DataTable.tsx` around lines 407 - 410, Update the useVirtual condition in DataTable so virtualization is evaluated against the full sortedData collection rather than pagedData, and ensure pagination is disabled or bypassed when virtualization is active so the virtualized list can access the complete dataset. Preserve the existing enableVirtualization, enableExpanding, and virtualizationThreshold checks.
619-700: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCollapsed rows still render an extra
<tr>per data row.The expanded-row
TableRowis always mounted (only its innerCollapsecontent is unmounted), doubling the row count in the DOM and ingetAllByRole('row')results. Gate the whole row onexpandedto halve the rendered rows for expandable tables.♻️ Proposed change
- {enableExpanding && renderExpandedRow && ( + {enableExpanding && renderExpandedRow && expanded && (🤖 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 `@frontend/src/components/DataTable.tsx` around lines 619 - 700, Update renderDataRow so the expanded-row TableRow containing renderExpandedRow is rendered only when expanded is true, rather than always mounting it with a collapsed Collapse. Preserve the existing enableExpanding and renderExpandedRow guards and expanded content behavior.
500-524: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery resize
mousemovewrites tolocalStorage.
onMouseMoveupdatescolumnWidthson each event and the persistence effect (Lines 336-345) depends oncolumnWidths, so a single drag produces hundreds of synchronousJSON.stringify+localStorage.setItemcalls on the main thread. Debounce the persist write (or only persist widths onmouseup).Also, the listeners are attached for the component's whole lifetime and re-subscribed whenever the
columnsarray identity changes (e.g.PerformanceDashboardbuildsissueColumnsinline each render). Reading the column bounds from a ref and attaching listeners only whileresizingRef.currentis set avoids both.🤖 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 `@frontend/src/components/DataTable.tsx` around lines 500 - 524, Update the DataTable resize handling around the mousemove/mouseup effect and columnWidths persistence so dragging does not synchronously persist every width change: debounce the persistence write or persist the final widths on mouseup. Store the current column definitions or bounds in a ref, avoid depending on the columns array identity, and attach window listeners only for an active resize; ensure cleanup removes listeners and resets cursor/user-select state.
🤖 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 `@frontend/src/components/AuditLogDashboard.tsx`:
- Around line 487-494: Remove the persistKey prop from the DataTable in the
audit-log dashboard so typed User and IP Address filters are not persisted to
localStorage; leave the table’s other behavior and configuration unchanged.
In `@frontend/src/components/DataTable.tsx`:
- Around line 863-963: Fix the virtualized branch in DataTable by wrapping the
header columnheaders inside a child Box with role="row" within the existing
rowgroup. Replace the virtualized use of renderDataRow with a dedicated row
renderer that outputs Box elements with role="row" and role="cell", preserving
row content, styling, selection, and virtualization without rendering
TableRow/TableCell outside a table.
- Around line 412-417: Update the selection-resolution effect around selectedIds
to filter sortedData instead of raw data, using the same sorted/filtered
ordering and index basis used when generating row ids. Keep the
onSelectionChange payload unchanged, and document that enableRowSelection should
preferably be paired with a stable getRowId.
In `@frontend/src/components/PerformanceDashboard.tsx`:
- Line 25: Remove the unused SkeletonTable named import from the
PerformanceDashboard.tsx import statement, while preserving DataTable,
SkeletonCard, and DataTableColumn.
- Around line 419-491: Memoize the issueColumns definition in
PerformanceDashboard using React’s useMemo, with an appropriate dependency
array, so its identity remains stable across renders while preserving the
existing column configuration and cell behavior.
In `@frontend/src/components/TableFilter.tsx`:
- Around line 103-119: Update the clear IconButton in TableFilter’s selectValue
endAdornment to handle onMouseDown by stopping propagation before clearFilter
runs, while retaining onClick for the clear action.
In `@frontend/src/components/TablePagination.tsx`:
- Around line 72-89: Update the TablePagination component to generate a unique
rows-per-page identifier with React.useId(), then use that shared value for the
label’s htmlFor and the Select’s id instead of the hard-coded "rows-per-page"
string.
---
Nitpick comments:
In `@frontend/src/components/__tests__/TableSortLabel.test.tsx`:
- Around line 34-57: Extend the TablePagination test in the “renders range and
navigates pages” case to open the page-size Select, choose a different option,
and assert onRowsPerPageChange receives the selected value. Keep the existing
range and page-navigation assertions unchanged.
In `@frontend/src/components/DataTable.tsx`:
- Around line 407-410: Update the useVirtual condition in DataTable so
virtualization is evaluated against the full sortedData collection rather than
pagedData, and ensure pagination is disabled or bypassed when virtualization is
active so the virtualized list can access the complete dataset. Preserve the
existing enableVirtualization, enableExpanding, and virtualizationThreshold
checks.
- Around line 619-700: Update renderDataRow so the expanded-row TableRow
containing renderExpandedRow is rendered only when expanded is true, rather than
always mounting it with a collapsed Collapse. Preserve the existing
enableExpanding and renderExpandedRow guards and expanded content behavior.
- Around line 500-524: Update the DataTable resize handling around the
mousemove/mouseup effect and columnWidths persistence so dragging does not
synchronously persist every width change: debounce the persistence write or
persist the final widths on mouseup. Store the current column definitions or
bounds in a ref, avoid depending on the columns array identity, and attach
window listeners only for an active resize; ensure cleanup removes listeners and
resets cursor/user-select state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8679f496-cc10-4bc4-a003-baf6ca9d14ba
📒 Files selected for processing (9)
frontend/src/components/AuditLogDashboard.tsxfrontend/src/components/DataTable.tsxfrontend/src/components/PerformanceDashboard.tsxfrontend/src/components/TableFilter.tsxfrontend/src/components/TablePagination.tsxfrontend/src/components/TableSortLabel.tsxfrontend/src/components/__tests__/DataTable.test.tsxfrontend/src/components/__tests__/TableSortLabel.test.tsxfrontend/src/components/index.ts
| enableRowSelection | ||
| enableColumnResizing | ||
| enableColumnVisibility | ||
| enablePagination={false} | ||
| persistKey="audit-logs" | ||
| emptyMessage="No audit logs found" | ||
| aria-label="Audit logs" | ||
| /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
persistKey on the audit-log table stores user-typed PII in localStorage indefinitely.
DataTable persists the full filters map (see DataTable.tsx Lines 336-345). The columns here are filterable on User (email) and IP Address, so an operator's search terms — real user emails and IPs — get written to unencrypted browser storage with no expiry or clear-on-logout path. That's a retention/GDPR concern on an audit-log surface specifically.
Either drop persistKey here, or restrict persistence to non-PII state (sorting, column visibility/widths) and exclude filter values.
🤖 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 `@frontend/src/components/AuditLogDashboard.tsx` around lines 487 - 494, Remove
the persistKey prop from the DataTable in the audit-log dashboard so typed User
and IP Address filters are not persisted to localStorage; leave the table’s
other behavior and configuration unchanged.
| useEffect(() => { | ||
| if (!onSelectionChange) return; | ||
| const rows = data.filter((row, index) => selectedIds.has(getRowId(row, index))); | ||
| onSelectionChange(Array.from(selectedIds), rows); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [selectedIds, data, getRowId]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Row ids are derived from inconsistent index bases, so selection can map to the wrong rows.
Selection ids are generated from the sorted/filtered page position (page * rowsPerPage + index over sortedData, Line 620), but this effect resolves them against the raw data order (data.filter((row, index) => ...)). With the default index-based getRowId, any active sort, filter, or page change makes the two bases disagree and onSelectionChange reports the wrong rows. Resolving the ids against sortedData fixes the mismatch:
🐛 Proposed fix
- const rows = data.filter((row, index) => selectedIds.has(getRowId(row, index)));
+ const rows = sortedData.filter((row, index) => selectedIds.has(getRowId(row, index)));
onSelectionChange(Array.from(selectedIds), rows);
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [selectedIds, data, getRowId]);
+ }, [selectedIds, sortedData, getRowId]);Consider also documenting that a stable getRowId is strongly recommended whenever enableRowSelection is used.
📝 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.
| useEffect(() => { | |
| if (!onSelectionChange) return; | |
| const rows = data.filter((row, index) => selectedIds.has(getRowId(row, index))); | |
| onSelectionChange(Array.from(selectedIds), rows); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [selectedIds, data, getRowId]); | |
| useEffect(() => { | |
| if (!onSelectionChange) return; | |
| const rows = sortedData.filter((row, index) => selectedIds.has(getRowId(row, index))); | |
| onSelectionChange(Array.from(selectedIds), rows); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [selectedIds, sortedData, getRowId]); |
🤖 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 `@frontend/src/components/DataTable.tsx` around lines 412 - 417, Update the
selection-resolution effect around selectedIds to filter sortedData instead of
raw data, using the same sorted/filtered ordering and index basis used when
generating row ids. Keep the onSelectionChange payload unchanged, and document
that enableRowSelection should preferably be paired with a stable getRowId.
| {useVirtual ? ( | ||
| <Box role="table" aria-label={ariaLabel} sx={{ width: '100%' }}> | ||
| <Box | ||
| role="rowgroup" | ||
| sx={{ | ||
| display: 'flex', | ||
| borderBottom: 2, | ||
| borderColor: 'divider', | ||
| bgcolor: 'background.paper', | ||
| position: 'sticky', | ||
| top: 0, | ||
| zIndex: 1, | ||
| fontWeight: 600, | ||
| }} | ||
| > | ||
| {enableRowSelection && ( | ||
| <Box | ||
| role="columnheader" | ||
| sx={{ width: 48, p: 1, display: 'flex', alignItems: 'center' }} | ||
| > | ||
| <Checkbox | ||
| indeterminate={someVisibleSelected} | ||
| checked={allVisibleSelected} | ||
| onChange={toggleSelectAllVisible} | ||
| inputProps={{ 'aria-label': 'Select all rows on this page' }} | ||
| size="small" | ||
| /> | ||
| </Box> | ||
| )} | ||
| {visibleColumns.map((column) => { | ||
| const sortState = sorting.find((s) => s.id === column.id); | ||
| const sortIndex = sorting.findIndex((s) => s.id === column.id); | ||
| const width = getWidth(column); | ||
| return ( | ||
| <Box | ||
| key={column.id} | ||
| role="columnheader" | ||
| aria-sort={ | ||
| sortState | ||
| ? sortState.direction === 'asc' | ||
| ? 'ascending' | ||
| : 'descending' | ||
| : 'none' | ||
| } | ||
| sx={{ | ||
| width, | ||
| flex: `0 0 ${width}px`, | ||
| p: 2, | ||
| position: 'relative', | ||
| boxSizing: 'border-box', | ||
| }} | ||
| > | ||
| {enableSorting && column.sortable !== false ? ( | ||
| <TableSortLabel | ||
| label={column.header} | ||
| active={!!sortState} | ||
| direction={sortState?.direction ?? 'asc'} | ||
| sortIndex={sortIndex >= 0 ? sortIndex + 1 : undefined} | ||
| showSortIndex={sorting.length > 1} | ||
| onSort={(event) => handleSort(column.id, event)} | ||
| /> | ||
| ) : ( | ||
| column.header | ||
| )} | ||
| {enableColumnResizing && ( | ||
| <Box | ||
| role="separator" | ||
| aria-orientation="vertical" | ||
| aria-label={`Resize ${String(column.header)} column`} | ||
| onMouseDown={(event) => startResize(column.id, event)} | ||
| sx={{ | ||
| position: 'absolute', | ||
| right: 0, | ||
| top: 0, | ||
| height: '100%', | ||
| width: 6, | ||
| cursor: 'col-resize', | ||
| }} | ||
| /> | ||
| )} | ||
| </Box> | ||
| ); | ||
| })} | ||
| </Box> | ||
| {pagedData.length === 0 ? ( | ||
| <Box sx={{ p: 6, textAlign: 'center' }}> | ||
| <InboxIcon color="disabled" sx={{ fontSize: 48, mb: 1 }} /> | ||
| <Typography color="text.secondary">{emptyMessage}</Typography> | ||
| </Box> | ||
| ) : ( | ||
| <VirtualList | ||
| height={typeof maxHeight === 'number' ? maxHeight : 560} | ||
| itemCount={pagedData.length} | ||
| itemSize={rowHeight} | ||
| width="100%" | ||
| overscanCount={8} | ||
| > | ||
| {({ index, style }) => renderDataRow(pagedData[index], index, style)} | ||
| </VirtualList> | ||
| )} | ||
| </Box> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Virtualized branch produces an invalid ARIA/DOM table structure.
Two problems in this path:
- The header
Box role="rowgroup"containsrole="columnheader"children directly — ARIA requires arole="row"between them, so the sort state (aria-sort) is exposed on orphaned nodes. - The body renders
renderDataRow, which returns MUITableRow/TableCell(<tr>/<td>) as children of plain<div>s.<tr>outside a table context is invalid HTML and the implicit row/cell roles are dropped, so assistive tech sees arole="table"with no rows.
Wrapping the header cells in a role="row" element and rendering the virtualized rows as Box role="row" / role="cell" (rather than TableRow/TableCell) would make this path both valid and navigable.
🤖 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 `@frontend/src/components/DataTable.tsx` around lines 863 - 963, Fix the
virtualized branch in DataTable by wrapping the header columnheaders inside a
child Box with role="row" within the existing rowgroup. Replace the virtualized
use of renderDataRow with a dedicated row renderer that outputs Box elements
with role="row" and role="cell", preserving row content, styling, selection, and
virtualization without rendering TableRow/TableCell outside a table.
| Info as InfoIcon, | ||
| } from '@mui/icons-material'; | ||
| import { SkeletonCard, SkeletonTable } from './index'; | ||
| import { DataTable, SkeletonCard, SkeletonTable, type DataTableColumn } from './index'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the now-unused SkeletonTable import.
CI flags it as unused after the table migration.
🧹 Proposed fix
-import { DataTable, SkeletonCard, SkeletonTable, type DataTableColumn } from './index';
+import { DataTable, SkeletonCard, type DataTableColumn } from './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.
| import { DataTable, SkeletonCard, SkeletonTable, type DataTableColumn } from './index'; | |
| import { DataTable, SkeletonCard, type DataTableColumn } from './index'; |
🧰 Tools
🪛 GitHub Check: build-and-test
[warning] 25-25:
'SkeletonTable' is defined but never used
🤖 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 `@frontend/src/components/PerformanceDashboard.tsx` at line 25, Remove the
unused SkeletonTable named import from the PerformanceDashboard.tsx import
statement, while preserving DataTable, SkeletonCard, and DataTableColumn.
Source: Linters/SAST tools
| const issueColumns: DataTableColumn<PerformanceIssue>[] = [ | ||
| { | ||
| id: 'severity', | ||
| header: 'Severity', | ||
| accessorKey: 'severity', | ||
| filterType: 'select', | ||
| filterOptions: [ | ||
| { label: 'High', value: 'high' }, | ||
| { label: 'Medium', value: 'medium' }, | ||
| { label: 'Low', value: 'low' }, | ||
| ], | ||
| width: 120, | ||
| cell: ({ value }) => ( | ||
| <Chip | ||
| label={String(value)} | ||
| color={ | ||
| value === 'high' ? 'error' : value === 'medium' ? 'warning' : 'info' | ||
| } | ||
| size="small" | ||
| /> | ||
| ), | ||
| }, | ||
| { | ||
| id: 'type', | ||
| header: 'Type', | ||
| accessorKey: 'type', | ||
| filterType: 'text', | ||
| width: 160, | ||
| }, | ||
| { | ||
| id: 'description', | ||
| header: 'Description', | ||
| accessorKey: 'description', | ||
| filterType: 'text', | ||
| width: 280, | ||
| }, | ||
| { | ||
| id: 'actual', | ||
| header: 'Actual', | ||
| accessorKey: 'actual', | ||
| filterable: false, | ||
| align: 'right', | ||
| width: 120, | ||
| cell: ({ value }) => Number(value).toLocaleString(), | ||
| }, | ||
| { | ||
| id: 'threshold', | ||
| header: 'Threshold', | ||
| accessorKey: 'threshold', | ||
| filterable: false, | ||
| align: 'right', | ||
| width: 120, | ||
| cell: ({ value }) => Number(value).toLocaleString(), | ||
| }, | ||
| { | ||
| id: 'status', | ||
| header: 'Status', | ||
| accessorFn: (row) => row.actual / row.threshold, | ||
| filterable: false, | ||
| sortable: true, | ||
| width: 100, | ||
| align: 'center', | ||
| cell: ({ row }) => ( | ||
| <Tooltip title={`Ratio: ${(row.actual / row.threshold).toFixed(2)}`}> | ||
| {row.actual > row.threshold ? ( | ||
| <TrendingUpIcon color="error" /> | ||
| ) : ( | ||
| issues.map((issue, index) => ( | ||
| <TableRow key={index}> | ||
| <TableCell> | ||
| <Chip | ||
| label={issue.severity} | ||
| color={issue.severity === 'high' ? 'error' : issue.severity === 'medium' ? 'warning' : 'info'} | ||
| size="small" | ||
| /> | ||
| </TableCell> | ||
| <TableCell>{issue.type}</TableCell> | ||
| <TableCell>{issue.description}</TableCell> | ||
| <TableCell>{issue.actual.toLocaleString()}</TableCell> | ||
| <TableCell>{issue.threshold.toLocaleString()}</TableCell> | ||
| <TableCell> | ||
| <Tooltip title={`Ratio: ${(issue.actual / issue.threshold).toFixed(2)}`}> | ||
| {issue.actual > issue.threshold ? ( | ||
| <TrendingUpIcon color="error" /> | ||
| ) : ( | ||
| <TrendingDownIcon color="success" /> | ||
| )} | ||
| </Tooltip> | ||
| </TableCell> | ||
| </TableRow> | ||
| )) | ||
| <TrendingDownIcon color="success" /> | ||
| )} | ||
| </TableBody> | ||
| </Table> | ||
| </TableContainer> | ||
| </Tooltip> | ||
| ), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Memoize issueColumns.
Built inline in the render body, this array gets a new identity on every render, which defeats the React.memo wrapper on DataTable, invalidates its filteredData/sortedData memos, and re-subscribes its global resize listeners (DataTable.tsx Lines 500-524) on each pass. AuditLogDashboard already wraps its column defs in useMemo; do the same here.
♻️ Proposed change
- const issueColumns: DataTableColumn<PerformanceIssue>[] = [
+ const issueColumns: DataTableColumn<PerformanceIssue>[] = useMemo(() => [
{
id: 'severity',
...
- ];
+ ], []);📝 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.
| const issueColumns: DataTableColumn<PerformanceIssue>[] = [ | |
| { | |
| id: 'severity', | |
| header: 'Severity', | |
| accessorKey: 'severity', | |
| filterType: 'select', | |
| filterOptions: [ | |
| { label: 'High', value: 'high' }, | |
| { label: 'Medium', value: 'medium' }, | |
| { label: 'Low', value: 'low' }, | |
| ], | |
| width: 120, | |
| cell: ({ value }) => ( | |
| <Chip | |
| label={String(value)} | |
| color={ | |
| value === 'high' ? 'error' : value === 'medium' ? 'warning' : 'info' | |
| } | |
| size="small" | |
| /> | |
| ), | |
| }, | |
| { | |
| id: 'type', | |
| header: 'Type', | |
| accessorKey: 'type', | |
| filterType: 'text', | |
| width: 160, | |
| }, | |
| { | |
| id: 'description', | |
| header: 'Description', | |
| accessorKey: 'description', | |
| filterType: 'text', | |
| width: 280, | |
| }, | |
| { | |
| id: 'actual', | |
| header: 'Actual', | |
| accessorKey: 'actual', | |
| filterable: false, | |
| align: 'right', | |
| width: 120, | |
| cell: ({ value }) => Number(value).toLocaleString(), | |
| }, | |
| { | |
| id: 'threshold', | |
| header: 'Threshold', | |
| accessorKey: 'threshold', | |
| filterable: false, | |
| align: 'right', | |
| width: 120, | |
| cell: ({ value }) => Number(value).toLocaleString(), | |
| }, | |
| { | |
| id: 'status', | |
| header: 'Status', | |
| accessorFn: (row) => row.actual / row.threshold, | |
| filterable: false, | |
| sortable: true, | |
| width: 100, | |
| align: 'center', | |
| cell: ({ row }) => ( | |
| <Tooltip title={`Ratio: ${(row.actual / row.threshold).toFixed(2)}`}> | |
| {row.actual > row.threshold ? ( | |
| <TrendingUpIcon color="error" /> | |
| ) : ( | |
| issues.map((issue, index) => ( | |
| <TableRow key={index}> | |
| <TableCell> | |
| <Chip | |
| label={issue.severity} | |
| color={issue.severity === 'high' ? 'error' : issue.severity === 'medium' ? 'warning' : 'info'} | |
| size="small" | |
| /> | |
| </TableCell> | |
| <TableCell>{issue.type}</TableCell> | |
| <TableCell>{issue.description}</TableCell> | |
| <TableCell>{issue.actual.toLocaleString()}</TableCell> | |
| <TableCell>{issue.threshold.toLocaleString()}</TableCell> | |
| <TableCell> | |
| <Tooltip title={`Ratio: ${(issue.actual / issue.threshold).toFixed(2)}`}> | |
| {issue.actual > issue.threshold ? ( | |
| <TrendingUpIcon color="error" /> | |
| ) : ( | |
| <TrendingDownIcon color="success" /> | |
| )} | |
| </Tooltip> | |
| </TableCell> | |
| </TableRow> | |
| )) | |
| <TrendingDownIcon color="success" /> | |
| )} | |
| </TableBody> | |
| </Table> | |
| </TableContainer> | |
| </Tooltip> | |
| ), | |
| }, | |
| ]; | |
| const issueColumns: DataTableColumn<PerformanceIssue>[] = useMemo(() => [ | |
| { | |
| id: 'severity', | |
| header: 'Severity', | |
| accessorKey: 'severity', | |
| filterType: 'select', | |
| filterOptions: [ | |
| { label: 'High', value: 'high' }, | |
| { label: 'Medium', value: 'medium' }, | |
| { label: 'Low', value: 'low' }, | |
| ], | |
| width: 120, | |
| cell: ({ value }) => ( | |
| <Chip | |
| label={String(value)} | |
| color={ | |
| value === 'high' ? 'error' : value === 'medium' ? 'warning' : 'info' | |
| } | |
| size="small" | |
| /> | |
| ), | |
| }, | |
| { | |
| id: 'type', | |
| header: 'Type', | |
| accessorKey: 'type', | |
| filterType: 'text', | |
| width: 160, | |
| }, | |
| { | |
| id: 'description', | |
| header: 'Description', | |
| accessorKey: 'description', | |
| filterType: 'text', | |
| width: 280, | |
| }, | |
| { | |
| id: 'actual', | |
| header: 'Actual', | |
| accessorKey: 'actual', | |
| filterable: false, | |
| align: 'right', | |
| width: 120, | |
| cell: ({ value }) => Number(value).toLocaleString(), | |
| }, | |
| { | |
| id: 'threshold', | |
| header: 'Threshold', | |
| accessorKey: 'threshold', | |
| filterable: false, | |
| align: 'right', | |
| width: 120, | |
| cell: ({ value }) => Number(value).toLocaleString(), | |
| }, | |
| { | |
| id: 'status', | |
| header: 'Status', | |
| accessorFn: (row) => row.actual / row.threshold, | |
| filterable: false, | |
| sortable: true, | |
| width: 100, | |
| align: 'center', | |
| cell: ({ row }) => ( | |
| <Tooltip title={`Ratio: ${(row.actual / row.threshold).toFixed(2)}`}> | |
| {row.actual > row.threshold ? ( | |
| <TrendingUpIcon color="error" /> | |
| ) : ( | |
| <TrendingDownIcon color="success" /> | |
| )} | |
| </Tooltip> | |
| ), | |
| }, | |
| ], []); |
🤖 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 `@frontend/src/components/PerformanceDashboard.tsx` around lines 419 - 491,
Memoize the issueColumns definition in PerformanceDashboard using React’s
useMemo, with an appropriate dependency array, so its identity remains stable
across renders while preserving the existing column configuration and cell
behavior.
| endAdornment={ | ||
| selectValue ? ( | ||
| <InputAdornment position="end" sx={{ mr: 2 }}> | ||
| <IconButton | ||
| size="small" | ||
| aria-label={`Clear ${label} filter`} | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| clearFilter(); | ||
| }} | ||
| edge="end" | ||
| > | ||
| <ClearIcon fontSize="small" /> | ||
| </IconButton> | ||
| </InputAdornment> | ||
| ) : undefined | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
MUI v5 Select endAdornment IconButton click opens dropdown menu mousedown
💡 Result:
To prevent the MUI Select dropdown menu from opening when you click an IconButton placed within the endAdornment, you must call event.stopPropagation on the event handler [1][2]. Because the Select component is a composite element, clicking an interactive adornment often bubbles the event up to the Select container, triggering its default open behavior [1]. You can resolve this by adding an event handler to your IconButton, as shown below: <IconButton onClick={(event) => { event.stopPropagation; // Your custom click logic here }} > In some cases, particularly if the dropdown is still opening or if there are conflicts with mouse-down behaviors, applying stopPropagation specifically to the onMouseDown event is more effective: <IconButton onMouseDown={(event) => { event.stopPropagation; // Your custom click logic here }} > Additional Considerations: 1. Overlapping Icons: A common issue when using endAdornment is that it overlaps the default Select chevron icon [3][4][5]. If your adornment is purely visual or intended for tooltips, you can prevent interaction with the adornment itself by adding the CSS property pointer-events: none [4][6]. 2. Styling: If you are adding an IconButton to serve as an action (like clearing the field), ensure you adjust the layout of the Select (e.g., using padding or absolute positioning) to prevent the adornment from sitting directly underneath the native dropdown trigger area [4][5]. 3. Component Design: Note that the Material UI Select component's support for endAdornment is historically limited, and complex custom adornments may require manual positioning to avoid interference with the built-in functionality [3][4][5].
Citations:
- 1: https://stackoverflow.com/questions/75757913/clicks-on-iconbutton-inside-muis-select-is-not-handled
- 2: https://v7.mui.com/joy-ui/react-select/
- 3: [Select] open/close indicator icon issue when endAdornment is set mui/material-ui#45270
- 4: https://stackoverflow.com/questions/63047684/material-ui-select-menu-with-end-adornment
- 5: Select with endAdornment overlaps with arrow icon mui/material-ui#17799
- 6: The select component Icon is not clickable mui/material-ui#35869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package references =="
(
fd -a 'package.json$' . | sed 's#^\./##' | head -50
) || true
echo "== locate TableFilter == "
fd -a 'TableFilter\.tsx$' . || true
echo "== relevant package manager files =="
if [ -f package.json ]; then cat package.json; fi
echo "== inspect TableFilter =="
file="$(fd 'TableFilter\.tsx$' . | head -1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view compact || true
sed -n '1,180p' "$file"
fi
echo "== search Select usages with endAdornment =="
rg -n "endAdornment|<Select|<TextField" -g '*.tsx' -g '*.ts' . | head -200Repository: nexoraorg/chenaikit
Length of output: 13442
🌐 Web query:
MUI Select handleMouseDown onMouseDown event propagation open menu
💡 Result:
In Material UI (MUI), the Select component primarily relies on the onMouseDown event—rather than the onClick event—to trigger the opening of the dropdown menu [1][2]. This design choice was implemented to improve the perceived responsiveness of the component [1]. ### Event Propagation and Behavior Because the Select component listens for onMouseDown to handle the opening of the menu, standard onClick event handlers placed on interactive children (such as Buttons, Icons, or Chips) inside the Select display area or menu items may not fire as expected [3][4]. The onMouseDown event from the child element bubbles up, is intercepted by the Select component, and triggers the menu open/toggle action before the onClick event can be handled [3][4]. ### Preventing Menu Open on Child Interaction If you need to include interactive elements inside a Select that should not trigger the menu, you must stop the propagation of the mousedown event on those specific elements [3][4]. You can achieve this by adding onMouseDown to your child component and calling event.stopPropagation(): <IconButton onMouseDown={(event) => { event.stopPropagation; // Your custom logic here }} > ### Important Technical Context * Event Handling: The Select component intentionally ignores standard click-based triggers in favor of onMouseDown [1][2]. If you are writing automated tests (e.g., using React Testing Library), you must fire mouseDown events rather than click events to interact with the Select [2][5]. * Pointer Interactions: Recent updates (as of 2026) have further refined this behavior to support "single-pointer-cycle" selection [6]. This includes logic where a pointerdown event opens the menu, and subsequent movement (pointermove) and release (pointerup) actions determine whether an item is selected or the menu is dismissed, aligning with pointer cancellation requirements (WCAG 2.5.2) [7][6]. * Internal Safety: The internal handleMouseDown function has undergone various stability improvements, such as the addition of null guards for displayRef to prevent runtime crashes during transient UI state changes [8][9].
Citations:
- 1: [Select] Improve response, react to mouse down mui/material-ui#17978
- 2: [Select] No longer listens to click events mui/material-ui#18655
- 3: https://stackoverflow.com/questions/75757913/clicks-on-iconbutton-inside-muis-select-is-not-handled
- 4: https://stackoverflow.com/questions/59522767/react-material-ui-chips-ondelete-inside-select-is-not-working
- 5: https://stackoverflow.com/questions/64778616/how-to-programmatically-open-material-ui-select
- 6: [select] Support single-pointer-cycle selection and pointer cancellation mui/material-ui#48328
- 7: [Select] Pointer Cancellation (WCAG 2.5.2) failure mui/material-ui#45301
- 8: [Select] Guard display ref during mouse down mui/material-ui#48744
- 9: [Select]
handleMouseDownthrows "Cannot read properties of null (reading 'focus')" whendisplayRefisnullmui/mui-x#23019
🌐 Web query:
GitHub MUI Select source handleMouseDown endAdornment stopPropagation mousedown
💡 Result:
In Material UI, the Select component listens for the mousedown event to trigger the opening of the dropdown menu [1][2]. This behavior was implemented to improve responsiveness and match native behavior, but it can cause issues when you include interactive elements (like buttons or icons) within an endAdornment, as the Select component may intercept those events [1][3]. If you are trying to prevent the Select component from opening when clicking an element inside your endAdornment, you can call event.stopPropagation on the mousedown event within that element [3]. Example workaround: <IconButton onMouseDown={(event) => { event.stopPropagation; }} onClick={(event) => { // Your custom action here }} > Important considerations: 1. Interaction Hijacking: The Select component intentionally captures events to manage its state [1][4]. While stopping propagation works for mousedown, it may not resolve issues where the Select overlay or internal layout is interfering with the visibility or accessibility of your adornment [5][6]. 2. Layout Overlaps: Historically, endAdornment elements could overlap with the default Select arrow/chevron icon, making them difficult to click [5][7][6]. Recent updates (e.g., #48723) have focused on improving the layout to ensure the endAdornment sits properly alongside the chevron rather than over it [8]. Ensure you are using the latest version of MUI to benefit from these layout fixes [8]. 3. Recommended Alternative: For complex scenarios—such as having multiple interactive elements inside the input (like chips or multiple buttons)—the official MUI recommendation is often to use the Autocomplete component instead, as it is designed to handle more complex child interactions natively [3][4].
Citations:
- 1: [Select] No longer listens to click events mui/material-ui#18655
- 2: [Select][base] Fix flicker on click of controlled Select button mui/material-ui#37855
- 3: [Select] Unable to trigger click events on Chips mui/material-ui#18658
- 4: [Select] Can't fire a click event inside mui/material-ui#35488
- 5: [Select] open/close indicator icon issue when endAdornment is set mui/material-ui#45270
- 6: select TextField with endAdornment overlap with arrow_drop_down icon mui/material-ui#17720
- 7: Select with endAdornment overlaps with arrow icon mui/material-ui#17799
- 8: mui/material-ui@0182cde
Stop Select from opening when the clear icon is clicked.
Select opens on mousedown, while the clearFilter() logic is only in onClick, so clicking the clear icon can both open the dropdown and clear the value. Add onMouseDown before clearing so the click handled by onClick is only the clear action.
🐛 Proposed fix
<IconButton
size="small"
aria-label={`Clear ${label} filter`}
+ onMouseDown={(event) => event.stopPropagation()}
onClick={(event) => {
event.stopPropagation();
clearFilter();
}}
edge="end"
>📝 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.
| endAdornment={ | |
| selectValue ? ( | |
| <InputAdornment position="end" sx={{ mr: 2 }}> | |
| <IconButton | |
| size="small" | |
| aria-label={`Clear ${label} filter`} | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| clearFilter(); | |
| }} | |
| edge="end" | |
| > | |
| <ClearIcon fontSize="small" /> | |
| </IconButton> | |
| </InputAdornment> | |
| ) : undefined | |
| } | |
| endAdornment={ | |
| selectValue ? ( | |
| <InputAdornment position="end" sx={{ mr: 2 }}> | |
| <IconButton | |
| size="small" | |
| aria-label={`Clear ${label} filter`} | |
| onMouseDown={(event) => event.stopPropagation()} | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| clearFilter(); | |
| }} | |
| edge="end" | |
| > | |
| <ClearIcon fontSize="small" /> | |
| </IconButton> | |
| </InputAdornment> | |
| ) : undefined | |
| } |
🤖 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 `@frontend/src/components/TableFilter.tsx` around lines 103 - 119, Update the
clear IconButton in TableFilter’s selectValue endAdornment to handle onMouseDown
by stopping propagation before clearFilter runs, while retaining onClick for the
clear action.
| <Typography variant="body2" color="text.secondary" component="label" htmlFor="rows-per-page"> | ||
| {labelRowsPerPage} | ||
| </Typography> | ||
| <FormControl size="small" disabled={disabled}> | ||
| <Select | ||
| id="rows-per-page" | ||
| value={rowsPerPage} | ||
| onChange={handleRowsPerPageChange} | ||
| inputProps={{ 'aria-label': labelRowsPerPage }} | ||
| sx={{ minWidth: 72 }} | ||
| > | ||
| {rowsPerPageOptions.map((option) => ( | ||
| <MenuItem key={option} value={option}> | ||
| {option} | ||
| </MenuItem> | ||
| ))} | ||
| </Select> | ||
| </FormControl> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hard-coded rows-per-page id breaks when two tables render on the same page.
Duplicate DOM ids make the htmlFor association ambiguous. Derive it with React.useId() instead.
♻️ Proposed change
+ const selectId = `rows-per-page-${React.useId()}`;
...
- <Typography variant="body2" color="text.secondary" component="label" htmlFor="rows-per-page">
+ <Typography variant="body2" color="text.secondary" component="label" htmlFor={selectId}>
{labelRowsPerPage}
</Typography>
<FormControl size="small" disabled={disabled}>
<Select
- id="rows-per-page"
+ id={selectId}📝 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.
| <Typography variant="body2" color="text.secondary" component="label" htmlFor="rows-per-page"> | |
| {labelRowsPerPage} | |
| </Typography> | |
| <FormControl size="small" disabled={disabled}> | |
| <Select | |
| id="rows-per-page" | |
| value={rowsPerPage} | |
| onChange={handleRowsPerPageChange} | |
| inputProps={{ 'aria-label': labelRowsPerPage }} | |
| sx={{ minWidth: 72 }} | |
| > | |
| {rowsPerPageOptions.map((option) => ( | |
| <MenuItem key={option} value={option}> | |
| {option} | |
| </MenuItem> | |
| ))} | |
| </Select> | |
| </FormControl> | |
| const selectId = `rows-per-page-${React.useId()}`; | |
| <Typography variant="body2" color="text.secondary" component="label" htmlFor={selectId}> | |
| {labelRowsPerPage} | |
| </Typography> | |
| <FormControl size="small" disabled={disabled}> | |
| <Select | |
| id={selectId} | |
| value={rowsPerPage} | |
| onChange={handleRowsPerPageChange} | |
| inputProps={{ 'aria-label': labelRowsPerPage }} | |
| sx={{ minWidth: 72 }} | |
| > | |
| {rowsPerPageOptions.map((option) => ( | |
| <MenuItem key={option} value={option}> | |
| {option} | |
| </MenuItem> | |
| ))} | |
| </Select> | |
| </FormControl> |
🤖 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 `@frontend/src/components/TablePagination.tsx` around lines 72 - 89, Update the
TablePagination component to generate a unique rows-per-page identifier with
React.useId(), then use that shared value for the label’s htmlFor and the
Select’s id instead of the hard-coded "rows-per-page" string.
2 participants
Summary
This PR implements a reusable, feature-rich data table component with advanced sorting, filtering, virtualization, and state persistence. The new table has been integrated into the Performance Dashboard and Audit Log Dashboard to provide a more powerful and responsive data browsing experience.
Changes
DataTable.tsxwith support for:TableSortLabel.tsxTableFilter.tsxTablePagination.tsxPerformanceDashboardAuditLogDashboardValidation
DataTable.tsxafter a failed localpnpminstall caused by a full disk.feat/issue-163-data-table.Closes #163
Summary by CodeRabbit
New Features
Bug Fixes
Tests