ENG-706: Migrate datamap report table to Ant Design#7780
ENG-706: Migrate datamap report table to Ant Design#7780gilluminate wants to merge 17 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
cefd234 to
2b8837b
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2b8837b to
d3632d9
Compare
7c17d24 to
a0fc7c3
Compare
059e53a to
858dcdb
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
858dcdb to
9c333c6
Compare
598271b to
b8131e2
Compare
b8131e2 to
e4fea42
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e4fea42 to
64b0fd0
Compare
Replaces TanStack Table implementation with Ant Design table for the datamap report. Moves column settings components out of v2/ into a dedicated column-settings/ directory, extracts row grouping logic into groupDatamapRows.ts, and extracts table state into useDatamapReportTable hook. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
react-dnd HTML5 backend requires a DataTransfer object on drag events to initialize a drag session. Headless Chrome in CI doesn't auto-create one on synthetic events, so the drag was silently a no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
64b0fd0 to
c526868
Compare
There was a problem hiding this comment.
Code Review — PR 7780: Migrate datamap report table to Ant Design
Overall this is a well-structured migration. The decision to extract all table logic into useDatamapReportTable, implement grouping via flat rowKey/rowSpan metadata in groupDatamapRows, and decouple ColumnSettingsModal from TanStack's TableInstance are all clean improvements. The Cypress test updates are thorough.
Actionable issues
Bug (silent render of "null"/"undefined") — Several boolean columns (COOKIE_REFRESH, DOES_INTERNATIONAL_TRANSFERS, EXEMPT_FROM_PRIVACY_REGULATIONS, PROCESSES_PERSONAL_DATA, REQUIRES_DATA_PROTECTION_ASSESSMENTS, USES_COOKIES, USES_NON_COOKIE_ACCESS, USES_PROFILING) call String(value) without a null guard. When the API returns null for these fields the cell displays the literal text "null". Other columns in the same file already guard with value !== null && value !== undefined ? String(value) : "". See inline comment on DatamapReportTableColumns.tsx:366.
Type confusion (inverted parameter names) — DraggableColumnListItem's prop type declares moveColumn: (hoverIndex, dragIndex), but the function is called as moveColumn(dragIndex, hoverIndex) and the implementation in DraggableColumnList expects (dragIndex, hoverIndex). The runtime behavior is correct because both are number, but the inverted names are a future footgun. See inline comment on DraggableColumnListItem.tsx:16.
Stale column order on custom-field changes — The useEffect at useDatamapReportTable.tsx:252 suppresses columns and setColumnOrder from its deps. If a user's custom fields change after initial render, the column order won't be updated to include the new fields until groupBy or datamapReport also changes. See inline comment.
Minor / nits
InteractiveTextCell: redundantellipsisprop on the outerLinkText(innerTextalready handles it). See inline.groupDatamapRowsrowKey: includingidxin the key can cause unnecessary React reconciliation if the server returns rows in a different order on refetch. See inline.sticky -top-6: hardcoded offset is fragile if the surrounding layout changes. A clarifying comment explaining what it compensates for would help. See inline.CustomTable/columnKeygating: the conditional is correct but a brief comment on whycolumnKeyis only forwarded when amenuis present would aid readability. See inline.ColumnSettingsModaldep oncolumns: addingcolumnsto theinitialColumnsmemo deps is the right fix from the old version. Worth a manual smoke-test of the "apply saved report → open Edit Columns" flow to confirm no race betweencolumnsandsavedCustomReportIdupdates. See inline.
clients/admin-ui/src/features/common/table/column-settings/DraggableColumnListItem.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/datamap/reporting/DatamapReportTableColumns.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/common/table/cells/InteractiveTextCell.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/datamap/reporting/hooks/useDatamapReportTable.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/common/table/column-settings/ColumnSettingsModal.tsx
Show resolved
Hide resolved
Fix inverted moveColumn param names, add null guards for boolean column renders, remove redundant ellipsis prop, add missing prefixColumns memo dep, and add clarifying comment for columnKey gating. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Deployment failed with the following error: |
lucanovera
left a comment
There was a problem hiding this comment.
The new table looks great! I've tested all functionality. Only found one small bug I think we should fix: if you change the "Group by" you can't expand the +x tags anymore until you reload the page. Other than that everything else works correctly so I'll give the approve now
b9a2c5f to
b32e35c
Compare
|
@lucanovera I think that's fixed now, will you double check? https://fides-plus-nightly-git-gill-eng-706migrate-datama-2ea01a-ethyca.vercel.app/reporting/datamap |
Ticket ENG-706
Description Of Changes
Migrate the datamap report table from TanStack Table (via
FidesTableV2) to Ant Design'sTablecomponent. This also migrates column settings from the v2 directory to a shared location and replaces Formik form handling with Ant DesignFormfor column renaming.Code Changes
useDatamapReportTablehook fromDatamapReportTable.tsxto encapsulate all table state management (pagination, sorting, filtering, grouping, column configuration, custom reports)groupDatamapRowsutility for client-side row grouping with expand/collapse supportDraggableColumnListandDraggableColumnListItemcomponents undertable/column-settings/, replacing the v2 versionsDatamapReportTableColumns.tsxto use Ant Design column definitions withCustomColumnsTypeinstead of TanStack column helpersCustomTableandCustomTableHeaderCellin fidesui to support column-level dropdown menus via amenupropgetAntDropdownOverlaycommand for dropdown overlay selectorshandleExportby checking promise result directly instead of captured booleanItemTypesexport and unusedisExportReportErrorfrom hook returnSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works