feat: DH-23429: Create Deephaven TableView wrapper - #2735
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2735 +/- ##
==========================================
+ Coverage 51.39% 52.12% +0.73%
==========================================
Files 797 802 +5
Lines 45887 45998 +111
Branches 11733 11766 +33
==========================================
+ Hits 23582 23976 +394
+ Misses 22286 22002 -284
- Partials 19 20 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds reusable Spectrum TableView infrastructure for dashboard-style windowed lists and controlled IrisGrid state.
Changes:
- Adds windowed, sortable, resizable TableView components.
- Adds controlled IrisGrid sorting and quick-filter callbacks.
- Synchronizes filter input with external values and expands tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/iris-grid/src/IrisGrid.tsx |
Adds controlled sort and filter proposals. |
packages/iris-grid/src/IrisGrid.test.tsx |
Tests controlled and uncontrolled state. |
packages/iris-grid/src/FilterInputField.tsx |
Synchronizes externally updated values. |
packages/iris-grid/src/FilterInputField.test.tsx |
Tests input synchronization and cancellation. |
packages/components/src/spectrum/tableView/TableViewWrapper.tsx |
Wraps Spectrum TableView with viewport tracking. |
packages/components/src/spectrum/tableView/TableViewWrapper.scss |
Enables column resizers. |
packages/components/src/spectrum/tableView/TableViewNormalized.tsx |
Renders normalized keyed rows. |
packages/components/src/spectrum/tableView/TableView.tsx |
Builds windowed row collections. |
packages/components/src/spectrum/tableView/TableView.test.tsx |
Tests rows, resizing, and viewport reporting. |
packages/components/src/spectrum/tableView/index.ts |
Exports TableView components. |
packages/components/src/spectrum/index.ts |
Exposes the TableView module. |
packages/components/src/spectrum/collections.ts |
Documents the custom TableView export. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const bottom = Math.max( | ||
| top, | ||
| Math.min(itemCount - 1, top + visibleRowCount) | ||
| ); |
| requestSortsChange(sorts: readonly SortDescriptor[]): void { | ||
| const { isSortsControlled, onSortsChange } = this.props; | ||
| onSortsChange?.(sorts); | ||
| if (!isSortsControlled) { | ||
| this.updateSorts(sorts); | ||
| } |
| requestQuickFiltersChange(quickFilters: ReadonlyQuickFilterMap): void { | ||
| const { isQuickFiltersControlled, onQuickFiltersChange } = this.props; | ||
| onQuickFiltersChange?.(quickFilters); | ||
| if (!isQuickFiltersControlled) { | ||
| this.updateQuickFilters(quickFilters); | ||
| } |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
We should also have a TableViews example page in the styleguide, similar to the ListViews.tsx page.
There was a problem hiding this comment.
Why does this PR have IrisGrid changes? I don't think IrisGrid is being used at all from the Dashboards screen.
There was a problem hiding this comment.
Thanks for catching that. It looks like I had some changes stashed and accidentally included them in this PR. I’ve removed those changes and added a TableView page to the styleguide.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:67
TableViewNormalizedalways passesTABLE_ROW_HEIGHT(33), but Spectrum 3.47 uses compact rows of 33px at medium scale and 41px at large scale (including the row border). With a large-scale Provider, dividingscrollTopby 33 causesonViewportChangeto report progressively incorrect rows and can request the wrong data window. Use a scale-aware table row-height map, as the existing ListView integration does, and cover both Provider scales.
rowHeight={TABLE_ROW_HEIGHT}
packages/components/src/spectrum/tableView/TableViewWrapper.scss:3
- Spectrum 3.47 defines this 21px resizer as
display: flexwithjustify-content: center; forcing it toblockdisables that centering and shifts the 1px divider away from the actual column boundary. Preserve flex layout (or remove this override entirely).
display: block !important;
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:63
- This always supplies an action callback even when the consumer omitted
onAction. Spectrum therefore treats every row as actionable and installs press/Enter behavior, but activation silently does nothing, giving users a false interactive affordance. PassundefinedwhenonActionis absent.
onAction={handleAction}
packages/components/src/spectrum/tableView/TableView.tsx:6
- The column descriptor cannot mark any column as
isRowHeader, so every body cell remains a generic grid cell and Spectrum cannot associate a row with a label for assistive-technology navigation. AddisRowHeader?: booleanhere, forward it to<Column>, and mark the identifying column in consumers/examples.
export interface TableViewColumn {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:48
- Spectrum stringifies keys that are explicitly set on collection elements (the same behavior is handled in
useStringifiedMultiSelection.ts:36-42).TableViewuses numeric row indexes, so an action key such as"2"will not strictly equalcandidate.key === 2, andonActionis never invoked for those rows. Match the stringified forms when resolving the item.
const item = normalizedItems.find(candidate => candidate.key === key)
?.item;
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:67
- The viewport calculation always assumes a 33px row, but Spectrum's compact table row sizing changes with the provider scale. In
largescale, dividingscrollTopandclientHeightby the medium-scale height reports the wrong row range, so a windowed consumer can fetch the wrong rows or leave visible placeholders unloaded. Use a scale-aware table row height (or measure the rendered row) for this value.
rowHeight={TABLE_ROW_HEIGHT}
packages/components/src/spectrum/tableView/TableViewWrapper.scss:1
- This class is applied to the Spectrum table itself, but it never sizes the table to its container. Spectrum does not provide default fill dimensions, so the style-guide's fixed-height parent does not constrain the 24-row table into an internal scroll viewport; it can grow past the parent and
onViewportChangesees the content-sized body. Make the wrapper fill its containing panel, matching the existing list-view wrapper behavior.
.dh-table-view-wrapper {
packages/components/src/spectrum/tableView/TableViewWrapper.scss:5
- Spectrum's column resizer is a flex container so its 1px pseudo-element is centered within the 21px hit target. Forcing
blockremoves that centering and shifts the visible divider away from the actual column boundary. Preserve the flex display while overriding the hidden state.
display: block !important;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:58
bottomis documented and tested as inclusive, but this adds the row count rather thanvisibleRowCount - 1. For example, three row-heights starting at row 2 contain rows 2–4, yet the callback reports 2–5. Subtract one when calculating the final index and update the test expectation accordingly.
Math.min(itemCount - 1, top + visibleRowCount)
| const item = normalizedItems.find(candidate => candidate.key === key) | ||
| ?.item; |
| 'sample-section-dropdown-menus', | ||
| 'sample-section-navigations', | ||
| 'sample-section-list-views', | ||
| 'sample-section-table-views', |
mofojed
changed the title
|
@SimonVutov I created a subtask on the original Jira ticket specifically for this and updated the PR description. |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
I've not convinced we need these styles, but also things look really messed up in the styleguide right now:
Look at how ListViews does it and link it up style it similarly so each example is contained correctly instead of overlapping.
| padding: 1rem; | ||
| } | ||
|
|
||
| .style-guide-table-view { |
There was a problem hiding this comment.
We shouldn't need to add any new styles here.
| <Flex direction="column" gap={8}> | ||
| <Text>Resizable and sortable</Text> | ||
| <div className="style-guide-table-view"> | ||
| <TableView | ||
| aria-label="Resizable and sortable table" | ||
| columns={columns} | ||
| items={sortedRows} | ||
| itemCount={sortedRows.length} | ||
| sortDescriptor={sortDescriptor} | ||
| onSortChange={setSortDescriptor} | ||
| onAction={item => setLastAction(item.name)} | ||
| onViewportChange={handleViewportChange} | ||
| renderCell={renderCell} | ||
| getTextValue={item => item.name} | ||
| /> | ||
| </div> | ||
| <Text> | ||
| Last action: {lastAction}; visible rows: {viewport} | ||
| </Text> | ||
| </Flex> | ||
|
|
||
| <Flex direction="column" gap={8}> | ||
| <Text>Windowed data at offset 3 of 12 rows</Text> | ||
| <div className="style-guide-table-view style-guide-table-view-windowed"> | ||
| <TableView | ||
| aria-label="Windowed data table" | ||
| columns={columns} | ||
| items={rows.slice(3, 9)} | ||
| itemCount={12} | ||
| offset={3} |
There was a problem hiding this comment.
Use LabeledFlexContainers instead, check how ListViews lays out it's examples. Right now it looks like a mess.
| export interface TableViewProps<T> { | ||
| columns: TableViewColumn[]; | ||
| items: readonly T[]; | ||
| itemCount: number; | ||
| offset?: number; | ||
| sortDescriptor?: SortDescriptor; | ||
| onAction?: (item: T) => void; | ||
| onSortChange?: (descriptor: SortDescriptor) => void; | ||
| onViewportChange?: (top: number, bottom: number) => void; | ||
| renderCell: (item: T, columnKey: Key) => ReactNode; | ||
| getTextValue?: (item: T) => string; | ||
| renderEmptyState?: () => JSX.Element; | ||
| 'aria-label'?: string; | ||
| UNSAFE_className?: string; | ||
| } |
There was a problem hiding this comment.
We shouldn't be redefining these Props definitions, we should just extend the SpectrumTableViewProps, and use Omit to drop the props we don't want (like ListView does, or just use Pick to pick the props we want.
| {item == null ? null : ( | ||
| <div className="dh-table-view-cell" data-table-view-key={key}> | ||
| {renderCell(item, columnKey)} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
I don't think we need this wrapper div:
| {item == null ? null : ( | |
| <div className="dh-table-view-cell" data-table-view-key={key}> | |
| {renderCell(item, columnKey)} | |
| </div> | |
| )} | |
| {item == null ? null : renderCell(item, columnKey)} |
|
@SimonVutov you also need to add the missing e2e screenshots |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:54
- For a row-aligned viewport with a height of three rows and
top = 2, the inclusive visible range is2..4, but this reports2..5. This makes aligned viewports report/fetch an extra logical row. Compute the bottom asceil((scrollTop + clientHeight) / rowHeight) - 1(which also handles partial rows), and update the test expectation from(2, 5)to(2, 4).
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
| const item = normalizedItems.find(candidate => candidate.key === key) | ||
| ?.item; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:48
- Numeric row keys are stringified by React/Spectrum when assigned to
<Row key={key}>, so the defaultTableViewrows (whose keys are numbers) never match this strict comparison andonActionsilently does nothing. Compare normalized string keys (as the nearbyuseStringifiedMultiSelectionutility does) and add an action regression test with the default numeric keys.
const item = normalizedItems.find(candidate => candidate.key === key)
?.item;
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:63
- This always supplies an action handler, even when the consumer omitted
onAction. Spectrum consequently treats every row as actionable (including keyboard/click interaction), but activation is a no-op. Only pass the handler when an action was requested so non-action tables do not expose misleading interaction semantics.
onAction={handleAction}
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:54
- The reported
bottomis documented and tested as inclusive, but this calculation over-reports a row: with three exactly visible rows starting at 2, the inclusive range is[2, 4], not[2, 5]. Calculate the last visible index fromMath.ceil((element.scrollTop + element.clientHeight) / rowHeight) - 1(which also handles partial first rows), and update the test that currently asserts5.
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
| useEffect(() => { | ||
| if (scrollElement == null || onViewportChange == null) { | ||
| return undefined; | ||
| } | ||
| updateViewport(scrollElement); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/styleguide.spec.ts:29
- This new ID creates a
toHaveScreenshot('table-views.png')case below, but there are notable-views-*-linux.pngbaselines intests/styleguide.spec.ts-snapshots. The Playwright projects will therefore fail with missing snapshots. Please generate and commit the CI snapshot baselines for this section.
'sample-section-table-views',
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:63
handleActionis passed even when the optional consumer callback is absent. Spectrum treats the presence ofonActionas making every row actionable, so tables such as both new styleguide examples expose keyboard/click actions that do nothing. Preserve non-actionable row semantics by omitting the prop whenonActionis undefined.
onAction={handleAction}
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:67
- This hard-codes the medium-scale compact row height. Spectrum calculates compact TableView rows by provider scale (32px for medium and 40px for large, plus the row border), and this repository already selects scale-specific collection heights in
packages/jsapi-components/src/spectrum/ListView.tsx:36-37. Under a large-scale provider, dividingscrollTopby 33 reports the wrong viewport. Select the TableView row height fromuseSpectrumThemeProvider().scaleand add both scale values/tests.
rowHeight={TABLE_ROW_HEIGHT}
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:54
- The computed inclusive bottom index is off by one when the viewport begins on a row boundary: a three-row-high viewport starting at row 2 contains rows 2–4, but this reports 2–5 (as the new test currently expects). Compute the last intersecting row from
(scrollTop + clientHeight) / rowHeight, then subtract one and clamp it toitemCount - 1; this also handles partially visible rows correctly.
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:55
onViewportChangeis invoked for every native scroll/resize event, even when the computedtopandbottomare unchanged. This contradicts the prop contract and can repeatedly trigger viewport fetching while scrolling within a single row. Track the last emitted range in a ref and call the handler only when either boundary changes.
onViewportChange(top, bottom);
| interface LabeledProps extends BoxAlignmentStyleProps, StyleProps { | ||
| label: string; | ||
| children: ReactNode; | ||
| } | ||
|
|
||
| function LabeledFlexContainer({ | ||
| label, | ||
| children, | ||
| ...styleProps | ||
| }: LabeledProps): JSX.Element { | ||
| return ( | ||
| <Flex | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...styleProps} | ||
| direction="column" | ||
| gap={10} | ||
| minWidth={0} | ||
| > | ||
| <Text>{label}</Text> | ||
| {children} | ||
| </Flex> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
We should put this into its own file that exports the component and is used by both ListViews and TableViews, rather than duplicating in both files.
There was a problem hiding this comment.
We should have an example in the styleguide that triggers the onAction as well (which could just display an alert or something).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:45
- For an empty table,
itemCount - 1is-1, but the outerMath.max(top, ...)turns the range into(0, 0), reporting a row that does not exist. Skip viewport notifications whileitemCount <= 0so consumers do not request a phantom first row.
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:68
TABLE_ROW_HEIGHTis 33 px, but the installed React Spectrum 3.47.0 compact TableView layout uses 32 px at medium scale and 40 px at large scale. As a result, viewport indices drift from the actual rows (for example,scrollTop === 32still reports row 0), and large-scale themes are substantially further off. Derive this value from the active Spectrum scale using TableView's compact row heights instead.
rowHeight={TABLE_ROW_HEIGHT}
| <LabeledFlexContainer label="Windowed rows" height="100%" minWidth={0}> | ||
| <TableView | ||
| aria-label="Windowed rows" | ||
| columns={columns} | ||
| items={rows.slice(3)} | ||
| itemCount={12} | ||
| offset={3} | ||
| renderCell={renderCell} | ||
| /> | ||
| </LabeledFlexContainer> |
There was a problem hiding this comment.
This example is still very weird. We should just generate a list of a bunch of items (like ListView does with the icons list).
Also we should show onAction usage (like we do in ListViews).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:57
bottomis documented and tested as an inclusive index, but this adds the visible row count without subtracting one. With a 3-row-high viewport aligned at row 2, only rows 2–4 are visible while the callback reports 2–5. Compute the inclusive end fromscrollTop + clientHeight(and update the corresponding test expectation) so consumers do not request an extra row at every aligned scroll position.
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:25
- This narrows Spectrum's
renderEmptyStatecallback fromReactNodetoJSX.Element. As a result, the value picked intoTableViewPropsis not safely assignable here, and callers cannot use the existingTableViewEmptyState, whose return type isJSX.Element | null(packages/components/src/TableViewEmptyState.tsx:18). Preserve the Spectrum-compatible return type.
renderEmptyState?: () => JSX.Element;
packages/components/src/spectrum/tableView/TableViewNormalized.tsx:64
- A handler is passed to Spectrum even when the caller omitted
onAction. Spectrum uses the presence ofonActionto make rows actionable, so read-only tables (including the new “Windowed rows” sample) expose a misleading interactive row affordance that only no-ops. Passundefinedwhen no action was supplied.
onAction={handleAction}
packages/components/src/spectrum/tableView/TableView.test.tsx:95
- For a viewport exactly three row-heights tall starting at row 2, the inclusive visible range is 2–4, not 2–5. This expectation currently locks in the wrapper's off-by-one calculation instead of detecting it.
expect(onViewportChange).toHaveBeenLastCalledWith(2, 5);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:57
- When scrolling starts exactly on a row boundary, this reports one row beyond the inclusive visible range. For example, the added test's 3-row-high viewport at row 2 contains rows 2–4, but this calculation returns 5. Calculate the lower index from the viewport's absolute bottom edge and update the test expectation to 4; otherwise every aligned viewport unnecessarily requests an extra row.
const bottom = Math.max(
top,
Math.min(itemCount - 1, top + visibleRowCount)
);
packages/components/src/spectrum/tableView/TableViewWrapper.tsx:48
- For an empty table, the initial effect still reports
[0, 0]even though row 0 does not exist:itemCount - 1is-1, then the laterMath.max(top, ...)raises it back to 0. Skip viewport notifications when there are no rows so consumers do not request a nonexistent range.
if (onViewportChange == null || rowHeight == null) {
return;
| if (onViewportChange == null || rowHeight == null) { | ||
| return; | ||
| } |
| selectionMode="none" | ||
| sortDescriptor={sortDescriptor} | ||
| onSortChange={onSortChange} | ||
| onAction={handleAction} |
| onScroll={onScroll} | ||
| renderEmptyState={renderEmptyState} | ||
| itemCount={normalizedItems.length} | ||
| rowHeight={TABLE_ROW_HEIGHT} |
This should be merged before merging this change on iris: https://github.com/deephaven-ent/iris/pull/5120