feat(studio): drag-to-reorder experiment table columns with localStorage persistence [ASE-384] - #595
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds dnd-kit column reordering to DataView tables, persists column order per experiment group, updates an internal export, and adds the needed package dependencies. ChangesColumn reordering feature
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/packages/common/src/components/DataView/internal/TableContent.tsx`:
- Around line 316-321: `DragAlongCell` is replacing the normal body-cell
rendering path and dropping width/pinning/a11y/tooltip/subrow styling for every
visible cell. Update the `TableContent` body branch that renders
`SortableContext`/`DragAlongCell` so pinned columns and any cells that need the
full `TableDataCell` behavior continue to use the existing `TableDataCell` path,
while non-pinned draggable cells receive the same `getCellStyle`, `data-pinned`,
`headers`, `title`, and subrow/shadow class handling as `TableDataCell`. Also
thread the row context and `autoCellTooltips` through this branch so the body
matches the header behavior and preserves sticky pinning and alignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e59bd1e-1542-4aa8-9955-d5f52f4baf64
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
web/packages/common/package.jsonweb/packages/common/src/components/DataView/internal/TableColumnHeader.tsxweb/packages/common/src/components/DataView/internal/TableContent.tsxweb/packages/common/src/components/DataView/internal/index.tsweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx
💤 Files with no reviewable changes (1)
- web/packages/common/src/components/DataView/internal/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/packages/common/src/components/DataView/internal/TableContent.tsx (2)
462-469: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalid CSS
transitionfallback.
'width transform 0.2s ease-in-out'isn't valid transition shorthand (two property tokens in one segment) — the browser ignores it, so no fallback transition applies.🎨 Fix
- transition: transition ?? 'width transform 0.2s ease-in-out', + transition: transition ?? 'transform 0.2s ease-in-out',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/DataView/internal/TableContent.tsx` around lines 462 - 469, The default transition in TableContent’s cell style is invalid because the fallback string combines two property names in one transition segment, so it gets ignored by the browser. Update the style object in the TableContent cell rendering logic to use a valid CSS transition fallback, keeping the existing transition ?? fallback behavior but with a proper shorthand value that animates both width and transform correctly.
160-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter pinned ids out of
SortableContext. Pinned headers/cells render withoutuseSortable, soitemsno longer matches the mounted sortable nodes. Apply the same filter in the body branch too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/DataView/internal/TableContent.tsx` around lines 160 - 194, Filter pinned columns out of the SortableContext items in TableContent so the context only includes headers/cells that actually mount DraggableColumnHeader/useSortable. Update the header rendering branch where SortableContext consumes columnOrder, and apply the same pinned-id filtering in the body branch as well so the sortable item list stays aligned with mounted draggable nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/packages/common/src/components/DataView/internal/TableContent.tsx`:
- Around line 120-124: Add sortable keyboard support in the sensors setup inside
TableContent by updating the KeyboardSensor configuration to use
sortableKeyboardCoordinates as its coordinateGetter, and import
sortableKeyboardCoordinates from `@dnd-kit/sortable`. Keep the change localized to
the useSensors/useSensor block so keyboard-based column reordering works
correctly.
---
Outside diff comments:
In `@web/packages/common/src/components/DataView/internal/TableContent.tsx`:
- Around line 462-469: The default transition in TableContent’s cell style is
invalid because the fallback string combines two property names in one
transition segment, so it gets ignored by the browser. Update the style object
in the TableContent cell rendering logic to use a valid CSS transition fallback,
keeping the existing transition ?? fallback behavior but with a proper shorthand
value that animates both width and transform correctly.
- Around line 160-194: Filter pinned columns out of the SortableContext items in
TableContent so the context only includes headers/cells that actually mount
DraggableColumnHeader/useSortable. Update the header rendering branch where
SortableContext consumes columnOrder, and apply the same pinned-id filtering in
the body branch as well so the sortable item list stays aligned with mounted
draggable nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e78faf02-12ed-4a91-999f-1e4ba68d7e0b
📒 Files selected for processing (2)
web/packages/common/src/components/DataView/internal/TableColumnHeader.tsxweb/packages/common/src/components/DataView/internal/TableContent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/packages/common/src/components/DataView/internal/TableColumnHeader.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/packages/common/src/components/DataView/internal/TableColumnHeader.tsx (2)
43-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove dnd-kit attributes onto the grip button.
dragProps.attributesneeds to live on the same node asdragProps.listeners; putting it on<th>adds a stray tab stop/role to the header cell and leaves the actual handle without dnd-kit’s a11y props. ThreadsetActivatorNodeRefthrough and attach it to the button too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/DataView/internal/TableColumnHeader.tsx` around lines 43 - 69, The drag-and-drop accessibility props are attached to the wrong element in TableColumnHeader, leaving the grip button without dnd-kit semantics and adding unwanted behavior to the header cell. Move dragProps.attributes from the <th> onto the same button that already receives dragProps.listeners, and also wire dragProps.setActivatorNodeRef to that button alongside the existing drag handle setup. Keep the table header itself limited to layout/column props and use the grip button as the sole drag activator.
13-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
DraggableSyntheticListenersfordragProps.listeners.Record<string, unknown>is too broad for the<button {...dragProps.listeners} />spread and doesn’t match dnd-kit’s listener type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/DataView/internal/TableColumnHeader.tsx` around lines 13 - 32, Update TableColumnHeader’s DragProps so dragProps.listeners uses dnd-kit’s DraggableSyntheticListeners instead of Record<string, unknown>, and keep the button spread in TableColumnHeader type-safe. Locate the DragProps interface and the TableColumnHeader component, then adjust the listener prop type to match what dnd-kit provides so <button {...dragProps.listeners} /> is correctly typed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/packages/common/src/components/DataView/internal/TableColumnHeader.tsx`:
- Around line 43-69: The drag-and-drop accessibility props are attached to the
wrong element in TableColumnHeader, leaving the grip button without dnd-kit
semantics and adding unwanted behavior to the header cell. Move
dragProps.attributes from the <th> onto the same button that already receives
dragProps.listeners, and also wire dragProps.setActivatorNodeRef to that button
alongside the existing drag handle setup. Keep the table header itself limited
to layout/column props and use the grip button as the sole drag activator.
- Around line 13-32: Update TableColumnHeader’s DragProps so dragProps.listeners
uses dnd-kit’s DraggableSyntheticListeners instead of Record<string, unknown>,
and keep the button spread in TableColumnHeader type-safe. Locate the DragProps
interface and the TableColumnHeader component, then adjust the listener prop
type to match what dnd-kit provides so <button {...dragProps.listeners} /> is
correctly typed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a9b35e05-862c-446c-aa42-d12ce9ef7f53
📒 Files selected for processing (2)
web/packages/common/src/components/DataView/internal/TableColumnHeader.tsxweb/packages/common/src/components/DataView/internal/TableContent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/packages/common/src/components/DataView/internal/TableContent.tsx
de82577 to
76e9548
Compare
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…up [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…ce [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…AlongCell [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…vatorNodeRef [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
… setActivatorNodeRef [ASE-384]" This reverts commit 635f9ca. Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…yout [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…butes to grip button [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
76e9548 to
ded2b6c
Compare
…lumn reordering [ASE-384] Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Closes #ASE-384
Screen.Recording.2026-07-08.at.10.05.20.mov
Summary
Experiment table columns had no user-controllable ordering. This adds drag-and-drop column reordering to the Experiments table, with the order persisted to localStorage per experiment group so it survives page refreshes.
TanStack's
columnOrderstate was already fully wired through the DataView stack (useDataViewState→useCustomReactTable→useReactTable), so the table layer needed only a DnD interaction layer on top.What changed:
TableColumnHeader— accepts optionaldragProps({ attributes, listeners, isDragging }); renders aGripVerticalhandle when listeners are provided, separate from the sort button to avoid click conflicts.TableContent— newenableColumnReordering?: booleanprop (off by default, so no other DataViews are affected). When enabled: wraps the full table inDndContext(closestCenter + restrictToHorizontalAxis), renders header cells viaDraggableColumnHeader(useSortable + CSS.Translate to avoid squishing) and body cells viaDragAlongCellso column data animates with the header during drag. Pinned columns are excluded from DnD.ExperimentGroupDataView— opts in viaattributes.DataViewTableContent. Uses the existinguseLocalStoragehook keyed bynemo-studio:experiment-group-columns:${group.id}to seed and persist column order. Each experiment group has its own saved order.Test plan
Summary by CodeRabbit
enableColumnReorderingsetting to activate column reordering where supported.