From 0cc1d0496ae03f2d47af0ae8a7a19403e1771d54 Mon Sep 17 00:00:00 2001 From: Jaseem Jas Date: Tue, 1 Sep 2026 17:25:11 +0530 Subject: [PATCH 1/2] UN-4071 fix: resolve antd's nested dataIndex in the shadcn DataTable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LLM Whisperer API Keys table declares its Plan column with antd's documented nested-path form, `dataIndex: ["product", "name"]`, which real antd resolved to `record.product.name`. The shadcn adapter that replaced antd's Table only ever did a flat, single-key lookup, so it indexed the record with the array itself and JavaScript stringified that to the property name "product,name" — undefined for every row. The column declares no `render`, so the undefined went straight to the cell: no error, just a blank column. Resolve a path `dataIndex` by walking it, in both the cell lookup and the TanStack accessor. String `dataIndex` keeps `accessorKey` so TanStack's dotted-string deep access is unchanged. The `id` derivation is deliberately left alone: it stringifies an array to "product,name", the same spelling `columnKey` and `toSorterInfo` use, so normalising it in one place only would break sorter/filter matching for a nested column. This is the only nested dataIndex in either repo today, which is why neither the build nor the existing 74 DataTable tests caught it. --- .../src/components/data-table/DataTable.jsx | 37 +++++++++- .../components/data-table/DataTable.test.jsx | 74 +++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/data-table/DataTable.jsx b/frontend/src/components/data-table/DataTable.jsx index ddf6b17fad..d15b07753d 100644 --- a/frontend/src/components/data-table/DataTable.jsx +++ b/frontend/src/components/data-table/DataTable.jsx @@ -39,6 +39,24 @@ import { cn } from "@/lib/utils"; * import it rather than build their own. */ +/** + * One cell's value, read the way antd reads it. + * + * `dataIndex` is either a key or a PATH: `["product", "name"]` is antd's + * documented nested form and means `record.product.name`. The flat lookup this + * replaces indexed the record with the array itself, and JavaScript stringifies + * that to the property name `"product,name"` — so the value was always + * undefined, and silently so, because a column with no `render` hands it + * straight to the cell. LLMWhisperer's API Keys table declares its Plan column + * exactly that way and lost the whole column to a blank strip. + */ +function cellValue(record, dataIndex) { + if (Array.isArray(dataIndex)) { + return dataIndex.reduce((v, k) => (v == null ? undefined : v[k]), record); + } + return record?.[dataIndex]; +} + /** * One antd column → one TanStack column def. * @@ -76,7 +94,20 @@ function toColumn(c, path) { return { id, - accessorKey: c.dataIndex, + /* + * A path needs an accessor FUNCTION: TanStack's `accessorKey` is a single + * key (or a dotted string), so handing it the array would repeat the same + * stringified-`"product,name"` lookup one layer down. The string case keeps + * `accessorKey` deliberately — swapping it for `cellValue` too would drop + * TanStack's dotted-string deep access, a behaviour change beyond this fix. + * + * Nothing reads `row.getValue` today (the cell below resolves its own + * value, and `sortingFn` compares originals), so this is correctness for + * the value-based sorting or filtering a later change would reach for. + */ + ...(Array.isArray(c.dataIndex) + ? { accessorFn: (row) => cellValue(row, c.dataIndex) } + : { accessorKey: c.dataIndex }), header: c.title, enableSorting: Boolean(c.sorter), /* @@ -95,7 +126,9 @@ function toColumn(c, path) { : () => 0, meta, cell: ({ row }) => { - const value = c.dataIndex ? row.original?.[c.dataIndex] : undefined; + const value = c.dataIndex + ? cellValue(row.original, c.dataIndex) + : undefined; // antd's render(value, record, index) contract. return c.render ? c.render(value, row.original, row.index) : value; }, diff --git a/frontend/src/components/data-table/DataTable.test.jsx b/frontend/src/components/data-table/DataTable.test.jsx index d305594ffc..1cd676ad06 100644 --- a/frontend/src/components/data-table/DataTable.test.jsx +++ b/frontend/src/components/data-table/DataTable.test.jsx @@ -1521,3 +1521,77 @@ describe("DataTable controlled filters", () => { }); }); }); + +/** + * antd's `dataIndex` is either a key or a PATH — `["product", "name"]` reads + * `record.product.name`. The flat lookup this guards indexed the record with + * the array itself, which JavaScript stringifies to the property name + * `"product,name"`, so the value was always undefined and — because a column + * with no `render` hands it straight to the cell — silently blank. + * + * LLMWhisperer's API Keys table declares its Plan column exactly this way and + * lost the whole column to an empty strip after the migration. It is the only + * nested `dataIndex` in either repo, which is why nothing else caught it. + */ +describe("DataTable nested dataIndex", () => { + const nested = [ + { id: 1, product: { id: "free", name: "LLM Whisperer Free" } }, + ]; + + it("resolves an array dataIndex as a path into the record", () => { + render( + , + ); + expect(screen.getByText("LLM Whisperer Free")).toBeInTheDocument(); + }); + + it("renders an empty cell rather than throwing on a missing segment", () => { + expect(() => + render( + , + ), + ).not.toThrow(); + // Two rows, both with an empty Plan cell — not the empty state. + expect(document.querySelectorAll("tbody tr")).toHaveLength(2); + }); + + it("hands the resolved nested value to render, as antd does", () => { + const render_ = vi.fn((value) => `plan: ${value}`); + render( + , + ); + expect(render_).toHaveBeenCalledWith("LLM Whisperer Free", nested[0], 0); + expect(screen.getByText("plan: LLM Whisperer Free")).toBeInTheDocument(); + }); +}); From 25d24830e8cd50272000cf6b707d708bb6da9dee Mon Sep 17 00:00:00 2001 From: Jaseem Jas Date: Tue, 1 Sep 2026 17:35:49 +0530 Subject: [PATCH 2/2] UN-4071 fix: correct the accessor comment and pin the id invariant Self-review findings on the previous commit. The inline comment claimed "nothing reads row.getValue today", and that was wrong: TanStack defaults every column to sortUndefined: 1, and getSortedRowModel calls rowA.getValue() to apply it BEFORE consulting sortingFn. So the accessor half is live behaviour, not deferred correctness. Checked what that actually costs. A nested column with a sorter now reorders undefined-valued rows to the end -- identically to how an equivalent string column already does; verified both by probe, same order in and out. So the quirk is pre-existing and cross-cutting rather than introduced here, and no nested column declares a sorter today. The comment now says that instead of denying it. Also documented the id/columnKey/toSorterInfo three-way agreement at the line it constrains. It was argued only in a commit message, and "normalise this stringified array" is exactly the tidy-up a later reader would attempt -- which would silently stop a nested column reporting its sort. Tests: assert the missing-segment cells are actually EMPTY rather than just present; add a deeper path with an array-index segment; add the keyless-nested onChange case that guards the id invariant above. Both new tests were mutation-checked -- each fails against exactly the mutant it describes and no other. Dropped the fleet-wide "only nested dataIndex in either repo" claim, which a leaf test file cannot verify. 598 tests pass, build clean. --- .../src/components/data-table/DataTable.jsx | 35 +++++++--- .../components/data-table/DataTable.test.jsx | 65 +++++++++++++++++-- 2 files changed, 85 insertions(+), 15 deletions(-) diff --git a/frontend/src/components/data-table/DataTable.jsx b/frontend/src/components/data-table/DataTable.jsx index d15b07753d..da80224549 100644 --- a/frontend/src/components/data-table/DataTable.jsx +++ b/frontend/src/components/data-table/DataTable.jsx @@ -71,6 +71,14 @@ function cellValue(record, dataIndex) { * version used would collide across levels. */ function toColumn(c, path) { + /* + * Three places spell this identity, and they must agree: here, `columnKey` + * (ColumnFilter.jsx) and `toSorterInfo` below, which matches on it to answer + * antd's `onChange`. A PATH `dataIndex` with no `key` coerces to + * `"product,name"` in all three, so they still agree — which is why this is + * deliberately NOT normalised to `dataIndex.join(".")`. Tidying it here alone + * would silently stop sorting and filtering reporting for a nested column. + */ const id = String(c.key ?? c.dataIndex ?? path); // `column` rides along so the header can render antd's filter affordance, // which is declared on the antd def and has no TanStack equivalent. @@ -96,17 +104,28 @@ function toColumn(c, path) { id, /* * A path needs an accessor FUNCTION: TanStack's `accessorKey` is a single - * key (or a dotted string), so handing it the array would repeat the same - * stringified-`"product,name"` lookup one layer down. The string case keeps - * `accessorKey` deliberately — swapping it for `cellValue` too would drop - * TanStack's dotted-string deep access, a behaviour change beyond this fix. + * key, so handing it the array repeats the same stringified-`"product,name"` + * lookup one layer down. + * + * This is NOT inert. TanStack defaults every column to `sortUndefined: 1`, + * and `getSortedRowModel` reads `row.getValue()` to apply it BEFORE it ever + * consults `sortingFn` — so undefined-valued rows sort to the end even + * though `sortingFn` is `() => 0`. A nested column was accidentally exempt + * while every one of its values was undefined; with a real accessor it now + * behaves exactly as an equivalent string column already does (verified: + * both reorder identically on a sparse column). That quirk is pre-existing + * and cross-cutting, not introduced here, and no nested column declares a + * `sorter` today. * - * Nothing reads `row.getValue` today (the cell below resolves its own - * value, and `sortingFn` compares originals), so this is correctness for - * the value-based sorting or filtering a later change would reach for. + * The string case keeps `accessorKey` because it is unchanged, not because + * its behaviour is right: TanStack deep-reads a DOTTED string while the + * cell below reads it as a literal key, which is antd's own reading. Those + * two disagree for `dataIndex: "a.b"`. Pre-existing on both halves, no + * literal dotted `dataIndex` exists in either repo, and reconciling it is a + * behaviour change beyond this fix. */ ...(Array.isArray(c.dataIndex) - ? { accessorFn: (row) => cellValue(row, c.dataIndex) } + ? { accessorFn: (record) => cellValue(record, c.dataIndex) } : { accessorKey: c.dataIndex }), header: c.title, enableSorting: Boolean(c.sorter), diff --git a/frontend/src/components/data-table/DataTable.test.jsx b/frontend/src/components/data-table/DataTable.test.jsx index 1cd676ad06..f2faccdbbd 100644 --- a/frontend/src/components/data-table/DataTable.test.jsx +++ b/frontend/src/components/data-table/DataTable.test.jsx @@ -1530,8 +1530,7 @@ describe("DataTable controlled filters", () => { * with no `render` hands it straight to the cell — silently blank. * * LLMWhisperer's API Keys table declares its Plan column exactly this way and - * lost the whole column to an empty strip after the migration. It is the only - * nested `dataIndex` in either repo, which is why nothing else caught it. + * lost the whole column to an empty strip after the migration. */ describe("DataTable nested dataIndex", () => { const nested = [ @@ -1571,12 +1570,64 @@ describe("DataTable nested dataIndex", () => { />, ), ).not.toThrow(); - // Two rows, both with an empty Plan cell — not the empty state. - expect(document.querySelectorAll("tbody tr")).toHaveLength(2); + // Two rows, both with an EMPTY Plan cell — not the empty state, and not a + // stand-in like "undefined" or "N/A" that a laxer resolver would print. + const cells = document.querySelectorAll("tbody td"); + expect(cells).toHaveLength(2); + for (const cell of cells) { + expect(cell).toHaveTextContent(""); + } + }); + + it("walks a path of any depth, including an array index", () => { + render( + , + ); + expect(screen.getByText("a@example.com")).toBeInTheDocument(); + }); + + /* + * The identity a nested column reports back through antd's `onChange`. + * + * With no `key`, the column id and `toSorterInfo`'s lookup both coerce the + * array to `"product,name"` — agreeing only because NEITHER normalises it. + * Normalising the id alone (to `"product.name"`, say) is exactly the tidy-up + * a later reader would attempt, and it would silently stop a nested column + * reporting its sort, with every other test here still green. + */ + it("reports a keyless nested column through onChange when sorted", async () => { + const onChange = vi.fn(); + render( + , + ); + await userEvent.click(screen.getByText("Plan")); + expect(onChange).toHaveBeenCalled(); + const sorter = onChange.mock.calls.at(-1)[2]; + expect(sorter.field).toEqual(["product", "name"]); + expect(sorter.order).toBe("ascend"); }); it("hands the resolved nested value to render, as antd does", () => { - const render_ = vi.fn((value) => `plan: ${value}`); + const renderCell = vi.fn((value) => `plan: ${value}`); render( { title: "Plan", dataIndex: ["product", "name"], key: "product_name", - render: render_, + render: renderCell, }, ]} dataSource={nested} rowKey="id" />, ); - expect(render_).toHaveBeenCalledWith("LLM Whisperer Free", nested[0], 0); + expect(renderCell).toHaveBeenCalledWith("LLM Whisperer Free", nested[0], 0); expect(screen.getByText("plan: LLM Whisperer Free")).toBeInTheDocument(); }); });