UN-4071 [FIX] Restore the Plan column on the LLM Whisperer API Keys page - #2270
Merged
Conversation
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.
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.
|
jaseemjaskp
marked this pull request as ready for review
September 1, 2026 12:12
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/components/data-table/DataTable.jsx | Correctly resolves array-form nested paths for cell rendering and TanStack accessors without changing string-key behavior. |
| frontend/src/components/data-table/DataTable.test.jsx | Adds comprehensive regression coverage for nested path resolution and existing adapter contracts. |
Reviews (1): Last reviewed commit: "UN-4071 fix: correct the accessor commen..." | Re-trigger Greptile
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
DataTableadapter antd's nesteddataIndexform (["product", "name"]), so it resolves as a path into the record rather than as a single key.Why
The cloud plugin's Plan column is unchanged by the migration — it is byte-identical before and after
An array
dataIndexis antd's documented nested-path form, and real antd resolved it torecord.product.name.Tablenow resolves to thisDataTable(antd-structure.tsx), whose antd→TanStack adapter only did a flat, single-key lookup:With
c.dataIndex === ["product", "name"], JavaScript stringifies the array to the property name"product,name"→undefined. The column declares norender, so thatundefinedgoes straight into the cell. Nothing throws — the columnidcomes fromc.key("product_name") — so it fails silently: a header with an empty strip under it.Backend is unaffected;
GET /api/v1/llmwhisperer/keys/still returnsproduct: {id, name}per key.This is the only nested
dataIndexin either repo (grep -rn 'dataIndex:\s*\[' frontend/src→ one hit, in the cloud plugin), which is why neither the build nor the existing 74DataTabletests caught it.How
frontend/src/components/data-table/DataTable.jsx:cellValue(record, dataIndex)helper — walks an arraydataIndexas a path, short-circuiting on a nullish segment; unchanged flat lookup for a string.accessorFnfor a path,accessorKeykept for a string (unchanged behaviour).Correction from self-review — the accessor half is NOT inert. The first commit's comment claimed "nothing reads
row.getValuetoday". That is wrong: TanStack defaults every column tosortUndefined: 1, andgetSortedRowModelcallsrowA.getValue()to apply it before it consultssortingFn. I probed what that actually costs: a nested column carrying asorternow sorts undefined-valued rows to the end — identically to how an equivalent string column already does (probed both; same order in, same order out). So the quirk is pre-existing and cross-cutting rather than introduced here, and no nested column declares asortertoday. The comment now states this instead of denying it.Worth flagging separately, not fixed here: that
sortUndefinedpre-pass meanssortingFn: () => 0is not the complete guarantee against local reordering that the existing comment above it implies, for anysorter: truecolumn with sparse values. Pre-existing and out of scope.idderivation attoColumnis deliberately untouched. It stringifies an array to"product,name"— the same spellingcolumnKey(ColumnFilter.jsx) andtoSorterInfouse. Normalising it in one place only would silently break sorter/filter matching for a nested column.frontend/src/components/data-table/DataTable.test.jsx: newdescribe("DataTable nested dataIndex")— resolves the path; renders empty rather than throwing on a missing segment; hands the resolved value torender(value, record, index).Can this PR break any existing features? If yes, please list possible items. If no, please explain why.
Low risk, and scoped by construction:
dataIndexpath is byte-equivalent —cellValuefalls through to the samerecord?.[dataIndex], and those columns keepaccessorKeyexactly as before. Every column in both repos except one is a string.undefined, so there is nothing working today for it to regress. The one observable knock-on —sortUndefinedordering on a nested sortable column — is described above; no nested column declares asorter.id/columnKey/toSorterInfospellings are unchanged, so sorting, filtering andonChangereporting are untouched.Verified: all 596 frontend tests pass (35 files), including the 74 pre-existing
DataTabletests;vite buildsucceeds; biome clean.Relevant Docs
TabledataIndexnested-path form — https://ant.design/components/table#columnRelated Issues or PRs
Dependencies Versions / Env Variables
Notes on Testing
The three new tests were written first and confirmed failing against the unpatched adapter —
renderreceivedundefinedwhere"LLM Whisperer Free"was expected — then passing after the change. Fixtures use the exact column definition from the cloud plugin and the exact payload shape the portal returns.Verified in the browser, not just by the build. The fix was synced into a dev namespace over the DevSpace HMR loop (
OSS_FRONTEND_PATHconfirmed pointing at this branch's worktree, and the pod'sDataTable.jsxconfirmed to containcellValue), then/llm-whisperer/<org-id>/api-keyswas loaded. The same API key that rendered a blank Plan cell before the fix (e68ce309-9794-4bf5-9a6e-30e0524736bb) now rendersLLM Whisperer Free. Read out of the live DOM:Also confirmed the page is genuinely the shadcn build and not a stale antd one (no antd stylesheet; wrapper is
ant-table-wrapper w-fullwith shadcn table classes), so this exercises the fixed adapter rather than an antd fallback.Both new guards were mutation-checked
Each fails against exactly the mutant it describes, and no other:
idnormalised todataIndex.join(".")intoColumnonlyreports a keyless nested column through onChange when sortedwalks a path of any depth, including an array indexScreenshots
Plan column rendering
LLM Whisperer Freefor keye68ce309-…on/llm-whisperer/<org-id>/api-keys— the same row that was blank before the fix. (Screenshot captured locally; happy to attach on request.)Checklist
I have read and understood the Contribution Guidelines.