Skip to content

UN-4124 [FIX] Implement antd's expandable API on the shared DataTable so nested cell values expand again #2288

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.

Sign up for GitHub

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

Merged
vishnuszipstack merged 3 commits into from
Sep 18, 2026
Merged

UN-4124 [FIX] Implement antd's expandable API on the shared DataTable so nested cell values expand again #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e59d02a
UN-4124 [FIX] Implement antd's expandable on the shared DataTable
vishnuszipstack Sep 17, 2026
d197cde
UN-4124 [FIX] Report expansion keys in the caller's own type
vishnuszipstack Sep 18, 2026
577970e
Merge remote-tracking branch 'origin/main' into UN-4124-datatable-exp…
vishnuszipstack Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
242 changes: 211 additions & 31 deletions frontend/src/components/data-table/DataTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,17 +413,167 @@
* order-less state.
*/
sortDirections,
/**
* antd's `expandable={{ expandedRowRender, expandedRowKeys, rowExpandable,
* showExpandColumn, onExpand, … }}` — a full-width extra row rendered under

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trailing … implies the full antd surface, but expandRowByClick, childrenColumnName (tree rows), indentSize, columnWidth, fixed, expandIconColumnIndex and expandedRowOffset are silently ignored — the same failure mode this PR exists to fix. Given the file's whole theme, please enumerate what IS honoured instead:

/**
 * antd `expandable`. Honoured: expandedRowRender, expandedRowKeys,
 * defaultExpandedRowKeys, rowExpandable, showExpandColumn, expandIcon,
 * onExpand, onExpandedRowsChange, expandedRowClassName. Tree rows,
 * expandRowByClick and column-placement options are not.
 * Declared so it doesn't fall into `...props` and onto the wrapper <div>.
 */

That also drops the HITL incident narrative — the review editor is a cloud-only plugin that OSS readers can't see, and the ticket ref would be the first UN-xxxx in OSS frontend source (grep -rln 'UN-[0-9]' frontend/src is empty today). Ticket belongs in the commit message.

* the record it belongs to.
*
* Declared for the same reason as `onRow`, `showHeader`, `scroll`,
* `bordered`, `locale` and `sortDirections` above, and it is the widest
* silent drop of the set: undeclared, the whole object fell into `...props`
* and onto the wrapper <div>, so `expandedRowRender` was never called and
* the table rendered as if the prop had not been passed. 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 (UN-4124).
*/
expandable,
...props
}) {
const empty = locale?.emptyText ?? emptyText;
const [sorting, setSorting] = React.useState([]);
const [selection, setSelection] = React.useState({});

const rows = React.useMemo(() => dataSource ?? [], [dataSource]);
const cols = React.useMemo(
() => toColumns(columns, rowSelection),
[columns, rowSelection],

/*
* Expansion, in antd's shape. Keys are compared as strings because that is

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

* what TanStack's `getRowId` (and so `row.id`) produces from whatever
* `rowKey` resolves to — a call-site numbering its rows `key: index` passes
* numbers, and `[0].includes("0")` is false.
*/
const expandedRowRender = expandable?.expandedRowRender;
const canExpand = typeof expandedRowRender === "function";
const controlledExpandedKeys = expandable?.expandedRowKeys;
const [ownExpandedKeys, setOwnExpandedKeys] = React.useState(
() => expandable?.defaultExpandedRowKeys ?? [],
);
const expandedKeys = React.useMemo(
() => new Set((controlledExpandedKeys ?? ownExpandedKeys).map(String)),
[controlledExpandedKeys, ownExpandedKeys],
);

const isRowExpandable = React.useCallback(
(record) =>
canExpand &&
(typeof expandable?.rowExpandable === "function"
? Boolean(expandable.rowExpandable(record))
: true),
[canExpand, expandable?.rowExpandable],
);

const onExpandCb = expandable?.onExpand;
const onExpandedRowsChange = expandable?.onExpandedRowsChange;
const toggleExpanded = React.useCallback(
(key, record, originalKey) => {
const willExpand = !expandedKeys.has(key);
/*
* Report the caller's own key values, never the normalized strings:
* normalization exists to match TanStack's `row.id` and must not leak
* out. A controlled caller that passed `[1]` and then tests
* `next.includes(1)` would never match `["1"]`.
*/
const source = controlledExpandedKeys ?? ownExpandedKeys;
const next = willExpand
? [...source, originalKey]
: source.filter((k) => String(k) !== key);
// A controlled `expandedRowKeys` belongs to the parent: report, never set.
if (controlledExpandedKeys === undefined) {
setOwnExpandedKeys(next);
}
onExpandCb?.(willExpand, record);
onExpandedRowsChange?.(next);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
},
[
expandedKeys,
controlledExpandedKeys,
ownExpandedKeys,
onExpandCb,
onExpandedRowsChange,
],
);

/*
* 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`

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* itself uses when the record carries no key.
*/
const originalRowKey = React.useCallback(
(record, index) => {
const raw =
typeof rowKey === "function" ? rowKey(record) : record?.[rowKey];
return raw === undefined || raw === null ? index : raw;
},
[rowKey],
);

/*
* antd hides the toggle column for `showExpandColumn: false` — the idiom for

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* a table driven entirely by its own controls, which is how HITL opens a
* cell's nested table from a button inside the cell.
*/
const showExpandColumn = canExpand && expandable?.showExpandColumn !== false;
const expandIcon = expandable?.expandIcon;
const cols = React.useMemo(() => {
const base = toColumns(columns, rowSelection);
if (!showExpandColumn) {
return base;
}
return [
{
id: "__expand",
header: () => null,
enableSorting: false,
size: 48,

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

cell: ({ row }) => {

Check warning on line 530 in frontend/src/components/data-table/DataTable.jsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Move this component definition out of the parent component and pass data as props. 


See more on https://sonarcloud.io/project/issues?id=Zipstack_unstract&issues=AaCy07bGCPjMrq0JiNFy&open=AaCy07bGCPjMrq0JiNFy&pullRequest=2288
const record = row.original;
if (!isRowExpandable(record)) {
return null;
}
const expanded = expandedKeys.has(row.id);
const onExpand = (event) => {
// The row itself may carry an onRow click handler.
event?.stopPropagation?.();
toggleExpanded(row.id, record, originalRowKey(record, row.index));
};
if (typeof expandIcon === "function") {
return expandIcon({

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expanded,
record,
onExpand: (_record, event) => onExpand(event),
});
}
return (
<button
type="button"
aria-expanded={expanded}
aria-label={expanded ? "Collapse row" : "Expand row"}
onClick={onExpand}
className="ant-table-row-expand-icon inline-flex size-5 items-center justify-center rounded border text-muted-foreground hover:text-foreground"
>
{expanded ? (
<ChevronDown className="size-3" />
) : (
<ChevronRight className="size-3" />
)}
</button>
);
},
},
...base,
];
}, [
columns,
rowSelection,
showExpandColumn,
expandIcon,
expandedKeys,
isRowExpandable,
toggleExpanded,
originalRowKey,

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on expandedKeys / toggleExpanded / isRowExpandable here means every toggle re-runs toColumns and gives every column's cell function a new identity. flexRender does createElement(cell, ctx) for function cells, so a new identity is a new component type → React unmounts and remounts every cell's subtree on each expand/collapse.

User-visible: Tab to the chevron, press Enter, focus drops to <body> (antd keeps it); any popover open inside a data cell closes on toggle. Both current callers already rebuild columns inline so it's not a regression for them, but the shim now guarantees it for every expandable caller however carefully they memoise.

Cheapest fix: hold the four in a ref and read ref.current inside the cell so the deps shrink to [columns, rowSelection, showExpandColumn, expandIcon]. Proper fix: TanStack's own state.expanded + row.getIsExpanded() / row.toggleExpanded(). Happy with either, or a follow-up ticket if you'd rather not widen this PR.

]);
const leaves = React.useMemo(() => leafColumns(columns), [columns]);

/*
Expand Down Expand Up @@ -858,35 +1008,65 @@
</TableCell>
</TableRow>
) : table.getRowModel().rows.length ? (
table.getRowModel().rows.map((row) => (
<TableRow
key={row.id}
data-state={row.getIsSelected() ? "selected" : undefined}
className={
typeof rowClassName === "function"
? rowClassName(row.original, row.index)
: rowClassName
}
{...(onRow ? onRow(row.original, row.index) : {})}
>
{row.getVisibleCells().map((cell) => (
<TableCell
key={cell.id}
className={cn(
cell.column.columnDef.meta?.align === "center" &&
"text-center",
cell.column.columnDef.meta?.align === "right" &&
"text-right",
)}
table.getRowModel().rows.map((row) => {
const expanded =
expandedKeys.has(row.id) && isRowExpandable(row.original);
return (
/*
* The expanded row is a SIBLING <tr>, not a nested one: a
* table row may only contain cells, so antd's full-width
* panel has to be its own row spanning every column.
*/
<React.Fragment key={row.id}>
<TableRow
data-state={row.getIsSelected() ? "selected" : undefined}
className={
typeof rowClassName === "function"
? rowClassName(row.original, row.index)
: rowClassName
}
{...(onRow ? onRow(row.original, row.index) : {})}
>
{flexRender(
cell.column.columnDef.cell,
cell.getContext(),
)}
</TableCell>
))}
</TableRow>
))
{row.getVisibleCells().map((cell) => (
<TableCell
key={cell.id}
className={cn(
cell.column.columnDef.meta?.align === "center" &&
"text-center",
cell.column.columnDef.meta?.align === "right" &&
"text-right",
)}
>
{flexRender(
cell.column.columnDef.cell,
cell.getContext(),
)}
</TableCell>
))}
</TableRow>
{expanded ? (
<TableRow
className={cn(
"ant-table-expanded-row hover:bg-transparent",
typeof expandable?.expandedRowClassName === "function"
? expandable.expandedRowClassName(
row.original,
row.index,
)
: expandable?.expandedRowClassName,
)}
>
<TableCell
colSpan={table.getVisibleLeafColumns().length}
className="p-2"
>
{expandedRowRender(row.original, row.index, 0, true)}

@chandrasekharan-zipstack chandrasekharan-zipstack Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</TableCell>
</TableRow>
) : null}
</React.Fragment>
);
})
) : (
<TableRow className="hover:bg-transparent">
<TableCell
Expand Down
Loading
Loading