UN-4124 [FIX] Implement antd's expandable API on the shared DataTable so nested cell values expand again - #2288
Conversation
The Ant Design removal (#1683 / #2212) replaced `<Table>` with the shared DataTable, which never implemented antd's `expandable` API. Undeclared, the whole object fell into `...props` and was spread onto the wrapper <div>, where React ignores it — so `expandedRowRender` was never called and the table rendered as if the prop had not been passed, with no console error. HITL's review editor is the visible casualty: an array or object inside a table cell shows a truncated JSON blob with an expand button beside it, and clicking that button did nothing at all, leaving nested values unreadable in Table view. Support `expandedRowRender`, controlled `expandedRowKeys` and uncontrolled `defaultExpandedRowKeys`, `rowExpandable`, `showExpandColumn`, `expandIcon`, `expandedRowClassName`, `onExpand` and `onExpandedRowsChange`. The panel is a sibling <tr> spanning every column, since a row may only contain cells. Keys are compared as strings because that is what TanStack's `getRowId` (and so `row.id`) produces: a call-site numbering its rows `key: index` passes numbers, and `[0].includes("0")` is false — a mismatch that would have hidden every expansion on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review feedback on #2288. `onExpandedRowsChange` built its argument from the internal `expandedKeys` set, which is normalized with `.map(String)` so it can match TanStack's `row.id`. That normalization leaked out: a controlled caller that passed `[1]` was handed back `["1"]`, and a parent testing `includes(1)` against it would never match. Normalization is now strictly internal. The reported array is built from the caller's own keys — `controlledExpandedKeys ?? ownExpandedKeys` — appending `originalRowKey()`, which reverses `getRowId`'s stringification by reading `rowKey` off the record directly. Collapse compares with `String(k) !== key` so it can filter a normalized id out of an un-normalized list without rewriting the survivors' types. Expansion itself is unaffected: `expandedKeys` is still a string Set, so matching behaves exactly as before. `defaultExpandedRowKeys` no longer stringifies on the way into state, since the memo normalizes anyway. Both directions are covered and mutation-checked: stringifying the appended key fails the expand test, and filtering the normalized set fails the collapse test. Also addresses SonarCloud S9020 — `waitFor` + `getByText` becomes `findByText` in the nested-panel test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Reviewed alongside Zipstack/unstract-cloud#1778. The controlled/uncontrolled contract, key-type preservation and sibling-<tr> rendering all look right and are well pinned by the tests. Inline comments below — the two I'd treat as blocking are the customer name in the OSS test and the rowKey regression test that can't currently fail; the rest are one-liners or follow-ups.
| * HITL's review editor drives expansion from a button inside the cell, not | ||
| * from antd's toggle column, so it passes `showExpandColumn: false` and a | ||
| * controlled `expandedRowKeys` of row indices. The shim dropped `expandable` | ||
| * entirely, so the panel never rendered: a customer reviewing a MARS |
There was a problem hiding this comment.
Blocking: this names a customer ("MARS") in the open-source repo, and the fixture below (vendor: "Fruta", sap_mapping, hitl_flag, testResults) reads as lifted from a real document. Please genericise both the comment and the fixture keys/values.
While here, the comment can be one line — the test is self-describing:
// Call-site drives expansion from its own controls: showExpandColumn:false + controlled expandedRowKeys of row indices.| * and no way to read the nested values in Table view (UN-4124). | ||
| */ | ||
| describe("Table expandable (UN-4124)", () => { | ||
| const row = { |
There was a problem hiding this comment.
This fixture has no id field, so with the default rowKey="id" getRowId falls back to the index — "0" — which happens to equal String(key). The harness therefore passes whether or not rowKey="key" is honoured, i.e. it can't catch the exact regression the cloud PR fixes. Add an id: "INV-2043" (or similar) to the row so the default keying would actually break it.
| render( | ||
| <DataTable | ||
| columns={columns} | ||
| dataSource={[{ key: 0, name: "Row 1" }]} |
There was a problem hiding this comment.
Same as in antd-structure.test.jsx: this row has no id, so String(row?.id ?? index) is "0" regardless of rowKey. Give it an id that differs from key so the test only passes when rowKey="key" is respected.
| id: "__expand", | ||
| header: () => null, | ||
| enableSorting: false, | ||
| size: 48, |
There was a problem hiding this comment.
size is never read — the only width the shim applies is header.column.columnDef.meta?.width (see the <TableHead> style further down), so this is dead config of exactly the kind this PR is fixing. Suggest:
meta: { width: expandable?.columnWidth ?? 48 },which also picks up antd's columnWidth sub-key. Harmless under auto layout (LinkedPromptsSection), but the column would take an equal share under tableLayout="fixed".
| /* | ||
| * The untouched key behind `row.id`. `getRowId` stringifies whatever `rowKey` | ||
| * resolves to, so this is the only way back to the value the call-site | ||
| * actually holds — falling back to the row index, which is what `getRowId` |
There was a problem hiding this comment.
This claim is only true for a string rowKey. getRowId does String(rowKey(row)) for the function form with no index fallback, so the two disagree: a function returning undefined gives every row the id "undefined", while this reports the index — the parent echoes [0] back, "0" !== "undefined", and the toggle looks dead.
One-liner fix in getRowId: String(rowKey(row) ?? index). Then the comment here is accurate and can shrink to // Caller's un-stringified key; getRowId stringifies, so row.id can't be handed back.
| [columns, rowSelection], | ||
|
|
||
| /* | ||
| * Expansion, in antd's shape. Keys are compared as strings because that is |
There was a problem hiding this comment.
Nit: [0].includes("0") describes a bug that no longer exists in this code (it's a Set of strings now). Reads fine as one line: // Compared as strings: getRowId stringifies, so a caller's key 0 is row.id "0".
| ); | ||
|
|
||
| /* | ||
| * antd hides the toggle column for `showExpandColumn: false` — the idiom for |
There was a problem hiding this comment.
Nit: references HITL from OSS again — rot risk since the plugin can change independently. // showExpandColumn:false = table driven by the caller's own controls. says the same thing.
| toggleExpanded(row.id, record, originalRowKey(record, row.index)); | ||
| }; | ||
| if (typeof expandIcon === "function") { | ||
| return expandIcon({ |
There was a problem hiding this comment.
antd hands a custom expandIcon { prefixCls, expanded, record, expandable, onExpand }. The standard antd recipe starts with if (!expandable) return <span className="...-spaced" />, which with this call shape renders the spacer for every row. Since the shim already returns null for non-expandable rows above, passing expandable: true here is enough.
| colSpan={table.getVisibleLeafColumns().length} | ||
| className="p-2" | ||
| > | ||
| {expandedRowRender(row.original, row.index, 0, true)} |
There was a problem hiding this comment.
Low: when expandedRowRender returns null (HITL does this when no column is expanded for the row) this still emits an empty <tr><td> strip. Matches antd, so fine to leave — but if you want to be kinder than antd, const content = expandedRowRender(...) and skip the row when content == null. Also className="p-2" duplicates shadcn TableCell's own default.
| * button did nothing whatsoever — nested values were unreadable in Table view | ||
| * (UN-4124). | ||
| */ | ||
| describe("DataTable expandable", () => { |
There was a problem hiding this comment.
Coverage is solid on the controlled path. Untested branches worth a cheap vi.fn() each: function rowKey (+ the originalRowKey round-trip), custom expandIcon (onExpand(record, e) contract), defaultExpandedRowKeys, expandedRowClassName fn + string (app CSS targets .ant-table-expanded-row, nothing pins it), onExpand(false, …) on collapse, stopPropagation vs an onRow.onClick spy, and that the expanded row does not receive onRow props. Also the rowExpandable test could assert the toggle button is hidden too, not just the panel.
The preamble block above duplicates the prop JSDoc + incident narrative; tests are self-describing, so I'd drop it, and drop (UN-4124) from the describe title in antd-structure.test.jsx.
3 participants
What
expandableAPI on the sharedDataTable, which was lost in the Ant Design removal ([FIX] Added missing constant #1683 / [P0-P4] Remove Ant Design from the OSS frontend (shadcn/ui + Midnight Bloom) #2212).expandedRowRender, controlledexpandedRowKeys, uncontrolleddefaultExpandedRowKeys,rowExpandable,showExpandColumn,expandIcon,expandedRowClassName,onExpand,onExpandedRowsChange.Why
DataTablenever declaredexpandable. Undeclared, the whole object fell into...propsand was spread onto the wrapper<div>, where React ignores it — soexpandedRowRenderwas never called and the table rendered exactly as if the prop had not been passed.How
<tr>spanning every column, since a<tr>may only contain cells.getRowId(and thereforerow.id) produces. A call-site numbering its rowskey: indexpasses numbers, and[0].includes("0")isfalse— a mismatch that would have hidden every expansion on its own, even with the rest of the implementation correct.Can this PR break any existing features? If yes, please list possible items. If no, please explain why.
expandableprop renders exactly as before — the new code paths are reached only whenexpandedRowRenderis supplied, which today is a prop nothing could act on.expandableis passed.Relevant Docs
TableexpandableAPI (the surface being matched).Related Issues or PRs
Dependencies Versions / Env Variables
Notes on Testing
DataTable.test.jsxand extendsantd-structure.test.jsx.cascade-and-affordances.test.jsx(a React 19defaultPropscheck) and are unrelated to this PR — every offender it names lives underfrontend/src/plugins/, i.e. the gitignored cloud plugin overlay copied in locally, not OSS source.Screenshots
...
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code