Skip to content

feat: DH-9378: Web Keyed Selection - #2736

Open
dgodinez-dh wants to merge 92 commits into
deephaven:mainfrom
dgodinez-dh:dag_KeyedSelection
Open

feat: DH-9378: Web Keyed Selection#2736
dgodinez-dh wants to merge 92 commits into
deephaven:mainfrom
dgodinez-dh:dag_KeyedSelection

Conversation

@dgodinez-dh

Copy link
Copy Markdown
Contributor

Implements keyed row selection for iris-grid.

  • selection is now an object with interface Selection
  • selection is either RangedSelection or KeyedSelection based on table attributes
  • render logic updated to support keys selecting multiple rows
  • Copy, Filter By Value, and Download CSV updated to snapshot with keys
    See ticket for test code in the test plan.

Copilot AI review requested due to automatic review settings August 18, 2026 16:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rows synchronously before creating pendingRows. 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

  • setSelection installs a settled selection but leaves lastSelection pointing at the previous transient state. This occurs when resolveKeyedSelection installs 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. Keep lastSelection synchronized 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 a finally block so both success and failure release it.

Comment thread packages/grid/src/Grid.tsx Outdated
Comment thread packages/grid/src/GridRendererTypes.ts
Copilot AI review requested due to automatic review settings August 18, 2026 17:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • moveCursorToPosition is also the core mouse path (GridSelectionMouseHandler calls 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 enter KeyedSelection's existing-selection/ctrl-toggle branch, causing rows to oscillate instead of producing one replacement range. Keep mouse gestures transient until onUp, 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 > 1 async 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 selectedRanges field from the pre-update selection. A normal ranged click therefore leaves direct consumers of grid.state.selectedRanges one 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 a finally block while allowing ownership of the successfully frozen table to transfer to TableSaver.
    packages/grid/src/Grid.tsx:1127
  • A directly installed selection is already committed, but lastSelection is 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. Update lastSelection together with selection.
  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. GridWidgetPlugin stores that callback in independent selectedRanges state, 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 applyFilter call 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);

Comment thread packages/iris-grid/src/KeyedSelection.ts Outdated
Comment thread packages/grid/src/Grid.tsx Outdated
Comment thread packages/iris-grid/src/IrisGrid.tsx
Copilot AI review requested due to automatic review settings August 18, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 commitMouseGesture rebuilds 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 lastSelection is left pointing at the prior value. This occurs when resolveKeyedSelection installs 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. Keep lastSelection synchronized here.
  setSelection(selection: Selection): void {
    this.setState({
      selection,
      selectedRanges: selectionToRanges(selection),
    });

packages/grid/src/Grid.tsx:1363

  • setFocusRow is used by Go To Row/Value and can target a row outside the loaded viewport. For KeyedSelection, withUpdatedRanges immediately reads that row via valueForCell; unloaded cells return undefined, 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's autoSelectColumn semantics. A grid configured with autoSelectColumn: true previously 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 keyed selectAll() 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 selectedKeyValues remains 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, and pluginState, while gridSel is 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 (often null) 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 publish selectionToRanges(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 reached TableSaver and close() is after the awaited call. This leaks a server-side table on each failed keyed export. Close filteredTable in a finally block around the freeze.

Copilot AI review requested due to automatic review settings August 18, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls valueForCell for out-of-viewport rows (which returns undefined), 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
  • setSelection installs the asynchronously resolved keyed selection but leaves lastSelection pointing at the pending version. The next modifier-click passes that stale object to commitMouseGesture; 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 RangedSelection drops the Grid theme semantics. With the documented autoSelectColumn mode, 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 keyed selectAll() 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 Selection contract and makes IrisGrid update the goto-row field for a multi-row selection. Return null unless 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 in try/finally so 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. GridWidgetPlugin stores this callback separately and consequently passes stale ranges to legacy table plugins. Invoke it with selectionToRanges(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);

Comment thread packages/iris-grid/src/KeyedSelection.ts
Comment thread packages/iris-grid/src/KeyedSelection.ts
Copilot AI review requested due to automatic review settings August 18, 2026 20:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • moveCursorToPosition is 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 enters KeyedSelection'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.setFocusRow invokes this before scrolling/fetching the destination row. For a keyed table, valueForCell therefore returns undefined for 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 leaves lastSelection pointing 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 synchronize lastSelection with 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. GridWidgetPlugin stores selectedRanges separately from selection, 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 calls onCancel, but this temporary table was never handed to TableSaver, so it cannot be closed there and leaks a live server table. Close the filtered copy in a finally block.

Comment thread packages/iris-grid/src/KeyedSelection.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants