feat: DH-9378: Web Keyed Selection - #2736
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 44 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
packages/iris-grid/src/KeyedSelection.ts:142
- The viewport clamp in the constructor does not protect this commit path: a shift selection spanning a very large table still pushes every row index into
rowssynchronously before creatingpendingRows. This can freeze the UI for millions or billions of rows. Determine cardinality and the first/last bounds from the ranges without enumerating every row, and only enumerate the single-row/toggle cases that need values immediately.
for (let i = 0; i < this.overlayRanges.length; i += 1) {
const { startRow, endRow } = this.overlayRanges[i];
if (startRow == null) continue; // eslint-disable-line no-continue
const last = endRow ?? startRow;
for (let r = startRow; r <= last; r += 1) {
packages/grid/src/Grid.tsx:1126
setSelectioninstalls a settled selection but leaveslastSelectionpointing at the previous transient state. This occurs whenresolveKeyedSelectioninstalls asynchronously fetched keys; a subsequent Ctrl+click consults the stale pending selection and may fail to deselect a key that was outside the old viewport. KeeplastSelectionsynchronized for this direct-set API.
this.setState({
selection,
selectedRanges: selectionToRanges(selection),
});
packages/iris-grid/src/sidebar/TableCsvExporter.tsx:306
- If
freeze()rejects,filteredTable.close()is skipped and the temporary server-side table leaks even though the outer catch cancels the download. Close the filtered table in afinallyblock so both success and failure release it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 45 changed files in this pull request and generated 3 comments.
Suppressed comments (8)
packages/grid/src/Grid.tsx:1490
moveCursorToPositionis also the core mouse path (GridSelectionMouseHandlercalls it on down and every drag update), so this commits keyed gestures before mouse-up. A normal keyed drag commits the first row immediately; subsequent drag updates then enterKeyedSelection's existing-selection/ctrl-toggle branch, causing rows to oscillate instead of producing one replacement range. Keep mouse gestures transient untilonUp, and commit explicitly only from keyboard/programmatic callers.
// Commit after every keyboard move so KeyedSelection resolves keys immediately.
this.commitSelection();
packages/iris-grid/src/KeyedSelection.ts:143
- This still materializes every row in the gesture before taking the
rows.length > 1async path. A shift-click or drag across a very large table therefore allocates/loops over the entire range on the UI thread, despite gesture-key rendering being viewport-bounded. Derive the pending range from the range endpoints without enumerating it, and asynchronously resolve multi-row gestures (including additive gestures).
packages/grid/src/Grid.tsx:1277 - As in the extend-selection branch, this derives the deprecated
selectedRangesfield from the pre-update selection. A normal ranged click therefore leaves direct consumers ofgrid.state.selectedRangesone gesture behind because the subsequent commit is an identity result.
selection: selection.withMouseGestureRanges(newRanges),
selectedRanges: selectionToRanges(selection),
packages/iris-grid/src/sidebar/TableCsvExporter.tsx:306
- If
freeze()rejects,filteredTable.close()is never reached, leaking the temporary server-side table. Close the filtered table in afinallyblock while allowing ownership of the successfully frozen table to transfer toTableSaver.
packages/grid/src/Grid.tsx:1127 - A directly installed selection is already committed, but
lastSelectionis left pointing at the prior (often pending) object. After async keyed-range resolution, the next ctrl-click is compared with the pending overlay; rows outside that overlay can no longer be toggled off correctly. UpdatelastSelectiontogether withselection.
setSelection(selection: Selection): void {
this.setState({
selection,
selectedRanges: selectionToRanges(selection),
});
packages/grid/src/Grid.tsx:1073
- Suppressing the deprecated callback for keyed selections leaves compatibility consumers with their previous ranged value.
GridWidgetPluginstores that callback in independentselectedRangesstate, so switching a widget from a ranged table to a keyed table can pass the old table's ranges to legacy plugins. Emit the compatibility representation ([]) for keyed selections as well.
if (isRangedSelection(selection)) {
onSelectionChanged(selection.toRanges());
}
packages/iris-grid/src/KeyedGridModel.ts:35
- The delimiter description is reversed: the implementation joins columns with tabs and rows with newlines. Correcting this exported interface documentation avoids giving API consumers the wrong wire format.
* Text version of snapshotByKeys: rows tab-separated, columns newline-separated.
packages/iris-grid/src/IrisGridTableModelTemplate.ts:1632
- Once the table copy is created, either
applyFiltercall can reject or time out, but the copy is only returned/closed on success. That leaks a server-side table on filter failures from copy, snapshot, and CSV paths. Close the copy before rethrowing when setup fails.
if (keyFilter != null) {
const filter = invertedSelection ? [keyFilter.not()] : [keyFilter];
await this.tableUtils.applyFilter(copy, filter);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (8)
packages/iris-grid/src/KeyedSelection.ts:158
- A shift-click can still synchronously iterate the entire selected row span here. The constructor now bounds preview-key enumeration to the viewport, but
commitMouseGesturerebuilds every row (potentially billions) before it reaches the pending async-resolution path, so releasing a large shift-selection can freeze the UI. Detect the multi-row range without materializing every row and defer it directly to the resolver; only enumerate rows for bounded gestures that must be toggled locally.
packages/grid/src/Grid.tsx:1126 - A directly installed selection is committed state, but
lastSelectionis left pointing at the prior value. This occurs whenresolveKeyedSelectioninstalls an asynchronously resolved shift-selection; the next Ctrl+click then compares against the stale pending selection and may fail to deselect an already-selected key. KeeplastSelectionsynchronized here.
setSelection(selection: Selection): void {
this.setState({
selection,
selectedRanges: selectionToRanges(selection),
});
packages/grid/src/Grid.tsx:1363
setFocusRowis used by Go To Row/Value and can target a row outside the loaded viewport. ForKeyedSelection,withUpdatedRangesimmediately reads that row viavalueForCell; unloaded cells returnundefined, so the grid commits a serialized undefined/null key and may highlight or copy the wrong key group even after the viewport loads. Defer keyed selection resolution until the target row's key values are available, as the shift-selection path does.
const newSel = state.selection.withUpdatedRanges([
new GridRange(null, focusedRow, null, focusedRow),
]);
packages/grid/src/Grid.tsx:1384
- Delegating Ctrl+A entirely to
Selection.selectAll()drops the grid theme'sautoSelectColumnsemantics. A grid configured withautoSelectColumn: truepreviously selected all columns using null row bounds;RangedSelection.selectAll()now always creates a full-row range, changing this documented grid mode. Preserve the theme-dependent bounds for ranged selections while retaining keyedselectAll()behavior.
selectAll(): void {
this.setState(state => {
const newSelection = state.selection.selectAll();
return {
selection: newSelection,
lastSelection: newSelection,
selectedRanges: selectionToRanges(newSelection),
packages/grid/src/mouse-handlers/GridSelectionMouseHandler.ts:250
- Right-clicking outside a keyed selection leaves the new row as an uncommitted gesture: right-button mouseup is ignored, so
selectedKeyValuesremains empty after the context menu closes. A later Ctrl+C or CSV export sees the highlighted selection but snapshots zero keys. Commit the selection after moving the cursor.
grid.clearSelectedRanges();
grid.moveCursorToPosition(column, row);
packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx:491
- The plugin element is memoized only by
Plugin,model, andpluginState, whilegridSelis read imperatively inside the memoized factory. Selection changes do not alter any memoization argument, so table plugins keep the selection captured when the element was first created (oftennull) and never receive keyed selection updates. Track the selection in panel state and include it in the memoized inputs, or remove this memoization for selection-dependent props.
const gridSel = this.irisGrid.current?.state.gridSelection;
const selectedRanges =
gridSel != null && isRangedSelection(gridSel) ? gridSel.toRanges() : [];
packages/grid/src/Grid.tsx:1074
- The deprecated callback is skipped for keyed selections. When a grid/model transitions from a ranged selection to keyed mode, existing consumers retain the previous ranges indefinitely (including
GridWidgetPlugin's compatibility state) instead of being cleared to[]. Always publishselectionToRanges(selection)to this callback; it already returns an empty array for keyed selections.
This issue also appears in the following locations of the same file:
- line 1122
- line 1361
- line 1378
const { onSelectionChanged, onSelectionChange } = this.props;
if (isRangedSelection(selection)) {
onSelectionChanged(selection.toRanges());
}
onSelectionChange(selection);
packages/iris-grid/src/sidebar/TableCsvExporter.tsx:306
- If
freeze()rejects, the newly created filtered table is never closed because ownership has not reachedTableSaverandclose()is after the awaited call. This leaks a server-side table on each failed keyed export. ClosefilteredTablein afinallyblock around the freeze.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 45 changed files in this pull request and generated 2 comments.
Suppressed comments (7)
packages/iris-grid/src/KeyedSelection.ts:159
- The preview enumeration is viewport-bounded, but committing still materializes every row in the full gesture range. A Ctrl+Shift selection retains
selectedKeys, so a large range takes this synchronous branch and callsvalueForCellfor out-of-viewport rows (which returnsundefined), both freezing the UI and producing incorrect keys. Route large/multi-row commits through asynchronous key resolution without constructing a per-row array.
packages/grid/src/Grid.tsx:1126 setSelectioninstalls the asynchronously resolved keyed selection but leaveslastSelectionpointing at the pending version. The next modifier-click passes that stale object tocommitMouseGesture; keys resolved outside the old viewport appear unselected there, so Ctrl+click cannot toggle them off correctly. A direct selection replacement must also establish the new committed baseline.
setSelection(selection: Selection): void {
this.setState({
selection,
selectedRanges: selectionToRanges(selection),
});
packages/grid/src/Grid.tsx:1388
- Delegating select-all to
RangedSelectiondrops the Grid theme semantics. With the documentedautoSelectColumnmode, this used to produce a full-column range (startRow/endRow = null); it now produces a full-row range, changing the ranges callback and scrollbar selection ticks for existing generic Grid consumers. Preserve the auto-select row/column bounds for ranged selections while using keyedselectAll()for keyed selections.
selectAll(): void {
this.setState(state => {
const newSelection = state.selection.selectAll();
return {
selection: newSelection,
lastSelection: newSelection,
selectedRanges: selectionToRanges(newSelection),
};
packages/iris-grid/src/KeyedSelection.ts:118
- A single selected key does not imply a single selected row when selection keys are non-unique—the keyed-table fixture intentionally maps one key to four rows. Returning the clicked row violates the
Selectioncontract and makesIrisGridupdate the goto-row field for a multi-row selection. Returnnullunless the model declares unique selection keys.
packages/iris-grid/src/IrisGridTableModelTemplate.ts:1633 - If filtering fails after the table copy is created, this method rejects without closing
copy; ownership never reaches a caller, so every failed keyed copy/filter/download leaks a JS API table. Close the copy on any error before rethrowing it.
const copy = await (this.table as DhType.Table).copy();
packages/iris-grid/src/sidebar/TableCsvExporter.tsx:306
- If
freeze()rejects,filteredTable.close()is never reached and the temporary filtered table leaks. Wrap the freeze intry/finallyso caller-owned resources are released on both success and failure.
packages/grid/src/Grid.tsx:1074 - The deprecated ranges callback is skipped for keyed selections, so a consumer that previously held ranged selection state never receives the compatibility value
[]when the grid switches to keyed selection.GridWidgetPluginstores this callback separately and consequently passes stale ranges to legacy table plugins. Invoke it withselectionToRanges(selection)for every selection type.
This issue also appears in the following locations of the same file:
- line 1122
- line 1381
const { onSelectionChanged, onSelectionChange } = this.props;
if (isRangedSelection(selection)) {
onSelectionChanged(selection.toRanges());
}
onSelectionChange(selection);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 45 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
packages/grid/src/Grid.tsx:1494
moveCursorToPositionis also used for mouse-down and every drag update (GridSelectionMouseHandler.ts:57,201), so this commits each transient keyed overlay. After the initial row is committed, the next drag entersKeyedSelection's existing-key toggle path; dragging from row 0 to row 1 leaves only row 1 selected instead of both, and later moves keep toggling groups. Keep mouse gestures transient until mouse-up, and commit explicitly only from keyboard call sites.
this.moveSelection(column, row, extendSelection, maximizePreviousRange);
// Commit after every keyboard move so KeyedSelection resolves keys immediately.
this.commitSelection();
packages/iris-grid/src/KeyedSelection.ts:270
Grid.setFocusRowinvokes this before scrolling/fetching the destination row. For a keyed table,valueForCelltherefore returnsundefinedfor an out-of-viewport destination, and Go To Row installs an[undefined]key that will not select the target after its data loads. Unfetched ranges need to become pending selections and use the async key resolver instead of being committed synchronously.
packages/grid/src/Grid.tsx:1125- The pending-key resolver calls
setSelection(resolved), but this leaveslastSelectionpointing at the pending overlay. The next Ctrl+click uses that stale object to decide whether the clicked key was selected, so a resolved key outside the pending overlay's cached viewport may not be deselected. A direct selection is already committed, so synchronizelastSelectionwith it.
setSelection(selection: Selection): void {
this.setState({
selection,
selectedRanges: selectionToRanges(selection),
packages/iris-grid/src/KeyedSelection.ts:24
- The sentinel encoding still aliases valid key values:
[NaN]and['__NaN__']both serialize to["__NaN__"](likewise for the infinity sentinels). Those distinct key groups then share one Set/Map entry and are selected or deselected together. Use an escaped, type-tagged encoding rather than values that can occur as ordinary strings.
packages/grid/src/Grid.tsx:1073 - Skipping the legacy callback for keyed selections leaves consumers' cached ranges stale.
GridWidgetPluginstoresselectedRangesseparately fromselection, so switching from a ranged table with a selection to a keyed table continues passing the old ranges to legacy plugins. Notify it with[]for keyed selections via the existing compatibility conversion.
This issue also appears in the following locations of the same file:
- line 1122
- line 1492
if (isRangedSelection(selection)) {
onSelectionChanged(selection.toRanges());
}
packages/iris-grid/src/sidebar/TableCsvExporter.tsx:306
- If
freeze()rejects,filteredTable.close()is skipped. The catch only callsonCancel, but this temporary table was never handed toTableSaver, so it cannot be closed there and leaks a live server table. Close the filtered copy in afinallyblock.
Implements keyed row selection for iris-grid.
See ticket for test code in the test plan.