[Unified Data Table] Prevent undefined errors when row accessed via…

… `rows[rowIndex]` (elastic#193791) ## Summary This PR fixes an issue present in 8.15, but which no longer exists after later refactoring, where saved search panels (possibly only ES|QL panels? It was hard to nail down since involved a race condition) could fail in dashboards when adding Unified Search filters that reduce the number of results in the grid. I'm still not 100% sure what the source of the issue was since it involved a race condition (didn't fail consistently for me locally) and internal EUI data grid code, but I have a hunch. The `undefined` errors occurred when trying to access a row by index from `DataTableContext` in certain cell renderers during the first render after search results had changed. I believe what was happening was the change to `DataTableContext` triggered a re-render of the cells before EUI data grid internally updated its state, resulting in attempts to access rows from within the cell renderers that no longer existed in the updated `DataTableContext`, and causing `undefined` reference errors. I'm making these changes in `main` instead of `8.15` directly because the updated approach is generally a safer way to retrieve rows and prevent similar issues in the future. Previously we added `rows` to `DataTableContext` and retrieved a single row by index using bracket notation, which can potentially return `undefined`, but TypeScript does not protect against it for us. Instead I've updated `DataTableContext` with a `getRowByIndex` method that correctly returns `DataTableRecord | undefined` and forces consumers to explicitly handle the `undefined` scenario. The PR will require some manual backporting to 8.15 since some things have changed since then, but it shouldn't be difficult. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine · Sep 24, 2024 · 6ff0791 · 6ff0791
1 parent 6bb9fbe
commit 6ff0791
Show file tree

Hide file tree

Showing 19 changed files with 153 additions and 108 deletions.
diff --git a/packages/kbn-unified-data-table/__mocks__/table_context.ts b/packages/kbn-unified-data-table/__mocks__/table_context.ts
@@ -15,18 +15,14 @@ import { servicesMock } from './services';
 import { DataTableContext } from '../src/table_context';
 import { convertValueToString } from '../src/utils/convert_value_to_string';
 import { buildDataTableRecord } from '@kbn/discover-utils';
-import type { DataTableRecord, EsHitRecord } from '@kbn/discover-utils/types';
+import type { DataTableRecord } from '@kbn/discover-utils/types';
 import type { UseSelectedDocsState } from '../src/hooks/use_selected_docs';
 
-const buildTableContext = (dataView: DataView, rows: EsHitRecord[]): DataTableContext => {
-  const usedRows = rows.map((row) => {
-    return buildDataTableRecord(row, dataView);
-  });
-
+const buildTableContext = (dataView: DataView, rows: DataTableRecord[]): DataTableContext => {
   return {
     expanded: undefined,
     setExpanded: jest.fn(),
-    rows: usedRows,
+    getRowByIndex: jest.fn((index) => rows[index]),
     onFilter: jest.fn(),
     dataView,
     isDarkMode: false,
@@ -38,16 +34,27 @@ const buildTableContext = (dataView: DataView, rows: EsHitRecord[]): DataTableCo
         rowIndex,
         columnId,
         fieldFormats: servicesMock.fieldFormats,
-        rows: usedRows,
+        rows,
         dataView,
         options,
       }),
   };
 };
 
-export const dataTableContextMock = buildTableContext(dataViewMock, esHitsMock);
+export const dataTableContextRowsMock = esHitsMock.map((row) =>
+  buildDataTableRecord(row, dataViewMock)
+);
+
+export const dataTableContextMock = buildTableContext(dataViewMock, dataTableContextRowsMock);
+
+export const dataTableContextComplexRowsMock = esHitsComplex.map((row) =>
+  buildDataTableRecord(row, dataViewComplexMock)
+);
 
-export const dataTableContextComplexMock = buildTableContext(dataViewComplexMock, esHitsComplex);
+export const dataTableContextComplexMock = buildTableContext(
+  dataViewComplexMock,
+  dataTableContextComplexRowsMock
+);
 
 export function buildSelectedDocsState(selectedDocIds: string[]): UseSelectedDocsState {
   const selectedDocsSet = new Set(selectedDocIds);

diff --git a/.../src/components/custom_control_columns/additional_row_control/row_control_column.test.tsx b/.../src/components/custom_control_columns/additional_row_control/row_control_column.test.tsx
@@ -54,6 +54,6 @@ describe('getRowControlColumn', () => {
 
     button.click();
 
-    expect(mockClick).toHaveBeenCalledWith({ record: contextMock.rows[1], rowIndex: 1 });
+    expect(mockClick).toHaveBeenCalledWith({ record: contextMock.getRowByIndex(1), rowIndex: 1 });
   });
 });
diff --git a/...table/src/components/custom_control_columns/additional_row_control/row_control_column.tsx b/...table/src/components/custom_control_columns/additional_row_control/row_control_column.tsx
@@ -26,7 +26,7 @@ export const RowControlCell = ({
 }: EuiDataGridCellValueElementProps & {
   renderControl: RowControlColumn['renderControl'];
 }) => {
-  const rowProps = useControlColumn(props);
+  const { record, rowIndex } = useControlColumn(props);
 
   const Control: React.FC<RowControlProps> = useMemo(
     () =>
@@ -50,17 +50,19 @@ export const RowControlCell = ({
                 color={color ?? 'text'}
                 aria-label={label}
                 onClick={() => {
-                  onClick?.(rowProps);
+                  if (record) {
+                    onClick?.({ record, rowIndex });
+                  }
                 }}
               />
             </EuiToolTip>
           </DataTableRowControl>
         );
       },
-    [props.columnId, rowProps]
+    [props.columnId, record, rowIndex]
   );
 
-  return renderControl(Control, rowProps);
+  return record ? renderControl(Control, { record, rowIndex }) : null;
 };
 
 export const getRowControlColumn = (

diff --git a/...components/custom_control_columns/additional_row_control/row_menu_control_column.test.tsx b/...components/custom_control_columns/additional_row_control/row_menu_control_column.test.tsx
@@ -66,6 +66,6 @@ describe('getRowMenuControlColumn', () => {
     expect(button).toBeInTheDocument();
 
     button.click();
-    expect(mockClick).toHaveBeenCalledWith({ record: contextMock.rows[1], rowIndex: 1 });
+    expect(mockClick).toHaveBeenCalledWith({ record: contextMock.getRowByIndex(1), rowIndex: 1 });
   });
 });
diff --git a/.../src/components/custom_control_columns/additional_row_control/row_menu_control_column.tsx b/.../src/components/custom_control_columns/additional_row_control/row_menu_control_column.tsx
@@ -34,7 +34,7 @@ export const RowMenuControlCell = ({
 }: EuiDataGridCellValueElementProps & {
   rowControlColumns: RowControlColumn[];
 }) => {
-  const rowProps = useControlColumn(props);
+  const { record, rowIndex } = useControlColumn(props);
   const [isMoreActionsPopoverOpen, setIsMoreActionsPopoverOpen] = useState<boolean>(false);
 
   const buttonLabel = i18n.translate('unifiedDataTable.grid.additionalRowActions', {
@@ -51,15 +51,17 @@ export const RowMenuControlCell = ({
             icon={iconType}
             color={color}
             onClick={() => {
-              onClick?.(rowProps);
+              if (record) {
+                onClick?.({ record, rowIndex });
+              }
               setIsMoreActionsPopoverOpen(false);
             }}
           >
             {label}
           </EuiContextMenuItem>
         );
       },
-    [rowProps, setIsMoreActionsPopoverOpen]
+    [record, rowIndex]
   );
 
   const popoverMenuItems = useMemo(
@@ -68,11 +70,11 @@ export const RowMenuControlCell = ({
         const Control = getControlComponent(rowControlColumn.id);
         return (
           <Fragment key={rowControlColumn.id}>
-            {rowControlColumn.renderControl(Control, rowProps)}
+            {record ? rowControlColumn.renderControl(Control, { record, rowIndex }) : null}
           </Fragment>
         );
       }),
-    [rowControlColumns, rowProps, getControlComponent]
+    [rowControlColumns, getControlComponent, record, rowIndex]
   );
 
   return (

diff --git a/...components/custom_control_columns/color_indicator/color_indicator_control_column.test.tsx b/...components/custom_control_columns/color_indicator/color_indicator_control_column.test.tsx
@@ -39,6 +39,6 @@ describe('ColorIndicatorControlColumn', () => {
         />
       </UnifiedDataTableContext.Provider>
     );
-    expect(getRowIndicator).toHaveBeenCalledWith(contextMock.rows[1], expect.any(Object));
+    expect(getRowIndicator).toHaveBeenCalledWith(contextMock.getRowByIndex(1), expect.any(Object));
   });
 });
diff --git a/packages/kbn-unified-data-table/src/components/data_table.tsx b/packages/kbn-unified-data-table/src/components/data_table.tsx
@@ -69,7 +69,7 @@ import {
   SELECT_ROW,
   OPEN_DETAILS,
 } from './data_table_columns';
-import { UnifiedDataTableContext } from '../table_context';
+import { DataTableContext, UnifiedDataTableContext } from '../table_context';
 import { getSchemaDetectors } from './data_table_schema';
 import { DataTableDocumentToolbarBtn } from './data_table_document_selection';
 import { useRowHeightsOptions } from '../hooks/use_row_heights_options';
@@ -629,11 +629,11 @@ export const UnifiedDataTable = ({
     );
   }, [currentPageSize, setPagination]);
 
-  const unifiedDataTableContextValue = useMemo(
+  const unifiedDataTableContextValue = useMemo<DataTableContext>(
     () => ({
       expanded: expandedDoc,
       setExpanded: setExpandedDoc,
-      rows: displayedRows,
+      getRowByIndex: (index: number) => displayedRows[index],
       onFilter,
       dataView,
       isDarkMode: darkMode,
@@ -876,7 +876,7 @@ export const UnifiedDataTable = ({
   const canSetExpandedDoc = Boolean(setExpandedDoc && !!renderDocumentView);
 
   const leadingControlColumns: EuiDataGridControlColumn[] = useMemo(() => {
-    const defaultControlColumns = getLeadControlColumns(canSetExpandedDoc);
+    const defaultControlColumns = getLeadControlColumns({ rows: displayedRows, canSetExpandedDoc });
     const internalControlColumns = controlColumnIds
       ? // reorder the default controls as per controlColumnIds
         controlColumnIds.reduce((acc, id) => {
@@ -907,6 +907,7 @@ export const UnifiedDataTable = ({
   }, [
     canSetExpandedDoc,
     controlColumnIds,
+    displayedRows,
     externalControlColumns,
     getRowIndicator,
     rowAdditionalLeadingControls,

diff --git a/packages/kbn-unified-data-table/src/components/data_table_columns.tsx b/packages/kbn-unified-data-table/src/components/data_table_columns.tsx
@@ -18,12 +18,13 @@ import {
 import { type DataView, DataViewField } from '@kbn/data-views-plugin/public';
 import { ToastsStart, IUiSettingsClient } from '@kbn/core/public';
 import { DocViewFilterFn } from '@kbn/unified-doc-viewer/types';
+import type { DataTableRecord } from '@kbn/discover-utils';
 import { ExpandButton } from './data_table_expand_button';
 import { CustomGridColumnsConfiguration, UnifiedDataTableSettings } from '../types';
 import type { ValueToStringConverter, DataTableColumnsMeta } from '../types';
 import { buildCellActions } from './default_cell_actions';
 import { getSchemaByKbnType } from './data_table_schema';
-import { SelectButton, SelectAllButton } from './data_table_document_selection';
+import { SelectButton, getSelectAllButton } from './data_table_document_selection';
 import {
   defaultTimeColumnWidth,
   ROWS_HEIGHT_OPTIONS,
@@ -73,18 +74,24 @@ const openDetails = {
   rowCellRender: ExpandButton,
 };
 
-const select = {
+const getSelect = (rows: DataTableRecord[]) => ({
   id: SELECT_ROW,
   width: DEFAULT_CONTROL_COLUMN_WIDTH,
   rowCellRender: SelectButton,
-  headerCellRender: SelectAllButton,
-};
+  headerCellRender: getSelectAllButton(rows),
+});
 
-export function getLeadControlColumns(canSetExpandedDoc: boolean) {
+export function getLeadControlColumns({
+  rows,
+  canSetExpandedDoc,
+}: {
+  rows: DataTableRecord[];
+  canSetExpandedDoc: boolean;
+}) {
   if (!canSetExpandedDoc) {
-    return [select];
+    return [getSelect(rows)];
   }
-  return [openDetails, select];
+  return [openDetails, getSelect(rows)];
 }
 
 function buildEuiGridColumn({

diff --git a/packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_json.tsx b/packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_json.tsx
@@ -11,20 +11,23 @@ import React, { useContext, useState } from 'react';
 import { FormattedMessage } from '@kbn/i18n-react';
 import { EuiContextMenuItem } from '@elastic/eui';
 import type { ToastsStart } from '@kbn/core/public';
+import type { DataTableRecord } from '@kbn/discover-utils';
 import { copyRowsAsJsonToClipboard } from '../utils/copy_value_to_clipboard';
 import { UnifiedDataTableContext } from '../table_context';
 
 interface DataTableCopyRowsAsJsonProps {
+  rows: DataTableRecord[];
   toastNotifications: ToastsStart;
   onCompleted: () => void;
 }
 
 export const DataTableCopyRowsAsJson: React.FC<DataTableCopyRowsAsJsonProps> = ({
+  rows,
   toastNotifications,
   onCompleted,
 }) => {
   const [isProcessing, setIsProcessing] = useState<boolean>(false);
-  const { rows, selectedDocsState, isPlainRecord } = useContext(UnifiedDataTableContext);
+  const { selectedDocsState, isPlainRecord } = useContext(UnifiedDataTableContext);
   const { getSelectedDocsOrderedByRows } = selectedDocsState;
 
   return (

diff --git a/packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_text.tsx b/packages/kbn-unified-data-table/src/components/data_table_copy_rows_as_text.tsx
@@ -12,23 +12,25 @@ import { uniq } from 'lodash';
 import { FormattedMessage } from '@kbn/i18n-react';
 import { EuiContextMenuItem } from '@elastic/eui';
 import type { ToastsStart } from '@kbn/core/public';
-import { calcFieldCounts } from '@kbn/discover-utils';
+import { DataTableRecord, calcFieldCounts } from '@kbn/discover-utils';
 import { copyRowsAsTextToClipboard } from '../utils/copy_value_to_clipboard';
 import { UnifiedDataTableContext } from '../table_context';
 
 interface DataTableCopyRowsAsTextProps {
+  rows: DataTableRecord[];
   toastNotifications: ToastsStart;
   columns: string[];
   onCompleted: () => void;
 }
 
 export const DataTableCopyRowsAsText: React.FC<DataTableCopyRowsAsTextProps> = ({
+  rows,
   toastNotifications,
   columns,
   onCompleted,
 }) => {
   const [isProcessing, setIsProcessing] = useState<boolean>(false);
-  const { valueToStringConverter, dataView, rows, selectedDocsState } =
+  const { valueToStringConverter, dataView, selectedDocsState } =
     useContext(UnifiedDataTableContext);
   const { isDocSelected } = selectedDocsState;
 

diff --git a/packages/kbn-unified-data-table/src/components/data_table_document_selection.test.tsx b/packages/kbn-unified-data-table/src/components/data_table_document_selection.test.tsx
@@ -15,9 +15,13 @@ import {
   DataTableCompareToolbarBtn,
   DataTableDocumentToolbarBtn,
   SelectButton,
-  SelectAllButton,
+  getSelectAllButton,
 } from './data_table_document_selection';
-import { buildSelectedDocsState, dataTableContextMock } from '../../__mocks__/table_context';
+import {
+  buildSelectedDocsState,
+  dataTableContextMock,
+  dataTableContextRowsMock,
+} from '../../__mocks__/table_context';
 import { UnifiedDataTableContext } from '../table_context';
 import { getDocId } from '@kbn/discover-utils';
 import { render, screen } from '@testing-library/react';
@@ -49,6 +53,7 @@ describe('document selection', () => {
       const contextMock = {
         ...dataTableContextMock,
       };
+      const SelectAllButton = getSelectAllButton(dataTableContextRowsMock);
 
       const component = mountWithIntl(
         <UnifiedDataTableContext.Provider value={contextMock}>
@@ -65,6 +70,7 @@ describe('document selection', () => {
         ...dataTableContextMock,
         selectedDocsState: buildSelectedDocsState(['i::1::']),
       };
+      const SelectAllButton = getSelectAllButton(dataTableContextRowsMock);
 
       const component = mountWithIntl(
         <UnifiedDataTableContext.Provider value={contextMock}>
@@ -197,7 +203,7 @@ describe('document selection', () => {
       const props = {
         isPlainRecord: false,
         isFilterActive: false,
-        rows: dataTableContextMock.rows,
+        rows: dataTableContextRowsMock,
         selectedDocsState: buildSelectedDocsState(['i::1::', 'i::2::']),
         setIsFilterActive: jest.fn(),
         enableComparisonMode: true,
@@ -242,7 +248,7 @@ describe('document selection', () => {
       const props = {
         isPlainRecord: false,
         isFilterActive: false,
-        rows: dataTableContextMock.rows,
+        rows: dataTableContextRowsMock,
         selectedDocsState: buildSelectedDocsState(['i::1::']),
         setIsFilterActive: jest.fn(),
         enableComparisonMode: true,
@@ -271,7 +277,7 @@ describe('document selection', () => {
       const props = {
         isPlainRecord: false,
         isFilterActive: false,
-        rows: dataTableContextMock.rows,
+        rows: dataTableContextRowsMock,
         selectedDocsState: buildSelectedDocsState(['i::1::', 'i::2::']),
         setIsFilterActive: jest.fn(),
         enableComparisonMode: true,
@@ -307,7 +313,7 @@ describe('document selection', () => {
       const props = {
         isPlainRecord: false,
         isFilterActive: false,
-        rows: dataTableContextMock.rows,
+        rows: dataTableContextRowsMock,
         selectedDocsState: buildSelectedDocsState(['i::1::', 'i::2::']),
         setIsFilterActive: jest.fn(),
         enableComparisonMode: true,
@@ -336,8 +342,8 @@ describe('document selection', () => {
       const props = {
         isPlainRecord: false,
         isFilterActive: false,
-        rows: dataTableContextMock.rows,
-        selectedDocsState: buildSelectedDocsState(dataTableContextMock.rows.map((row) => row.id)),
+        rows: dataTableContextRowsMock,
+        selectedDocsState: buildSelectedDocsState(dataTableContextRowsMock.map((row) => row.id)),
         setIsFilterActive: jest.fn(),
         enableComparisonMode: true,
         setIsCompareActive: jest.fn(),
@@ -357,7 +363,7 @@ describe('document selection', () => {
         </UnifiedDataTableContext.Provider>
       );
       expect(findTestSubject(component, 'unifiedDataTableSelectionBtn').text()).toBe(
-        `Selected${dataTableContextMock.rows.length}`
+        `Selected${dataTableContextRowsMock.length}`
       );
 
       expect(findTestSubject(component, 'dscGridSelectAllDocs').exists()).toBe(false);
@@ -368,7 +374,7 @@ describe('document selection', () => {
     const props = {
       isPlainRecord: false,
       isFilterActive: false,
-      rows: dataTableContextMock.rows,
+      rows: dataTableContextRowsMock,
       selectedDocsState: buildSelectedDocsState([]),
       setIsFilterActive: jest.fn(),
       enableComparisonMode: true,