Skip to content

Commit

Permalink
[8.17] [Security Solution] Adds callback onUpdatePageIndex to get c…
Browse files Browse the repository at this point in the history
…urrent `pageIndex` in Unified Data table (#201240) (#202348)

# Backport

This will backport the following commits from `main` to `8.17`:
- [[Security Solution] Adds callback `onUpdatePageIndex` to get current
`pageIndex` in Unified Data table
(#201240)](#201240)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jatin
Kathuria","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-29T15:14:27Z","message":"[Security
Solution] Adds callback `onUpdatePageIndex` to get current `pageIndex`
in Unified Data table (#201240)\n\n## Summary\n\nHandles resolution
for\n- Notes fetching data for all Timeline Records which leads
to\nperformance issues.\n-
#201330 \n\n## Issue - Notes
fetching data for all Timeline Records \n\nCurrently, there was no way
for consumer of `UnifiedDataGrid` to get the\ncurrent `pageIndex`.
Security Solution needs to get the current\n`pageIndex` so the items on
the current page can be calculated.\n\n@elastic/kibana-data-discovery ,
please let us know if you have any\nopinion here.\n\nThis results in
notes being fetched for all Timeline Records which means\nminimum of 500
records and if user has queries 5000 records ( for\nexample ), a request
will be made to query notes for all those 5000\nnotes which leads to
performance issue and sometimes error as
shown\nbelow:\n\n\n![image](https://github.com/user-attachments/assets/6fcfe05d-340c-4dcb-a273-5af53ed12945)\n\n\n##
👨‍💻 Changes\n\nThis adds attribute `pageIndex` to timeline state.
\n\n```javascript\n{\n \"pageIndex\": number\n}\n```\n`pageIndex` helps
with getting the events for that particular page.\n\n## 🟡 Caveat\n\n-
Currently this `pageIndex` is shared between Query and EQL tabs
which\ncan lead to wonky behavior at time.\n- Additionally, as of now
table maintains its own page index and\nconsumer component cannot effect
the `pageIndex` of the
UnifiedDataGrid.","sha":"de9d5465df5900936991d79306cb2cbbe63f4623","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["backport","release_note:fix","v9.0.0","Team:Threat
Hunting:Investigations","v8.16.2"],"number":201240,"url":"https://github.com/elastic/kibana/pull/201240","mergeCommit":{"message":"[Security
Solution] Adds callback `onUpdatePageIndex` to get current `pageIndex`
in Unified Data table (#201240)\n\n## Summary\n\nHandles resolution
for\n- Notes fetching data for all Timeline Records which leads
to\nperformance issues.\n-
#201330 \n\n## Issue - Notes
fetching data for all Timeline Records \n\nCurrently, there was no way
for consumer of `UnifiedDataGrid` to get the\ncurrent `pageIndex`.
Security Solution needs to get the current\n`pageIndex` so the items on
the current page can be calculated.\n\n@elastic/kibana-data-discovery ,
please let us know if you have any\nopinion here.\n\nThis results in
notes being fetched for all Timeline Records which means\nminimum of 500
records and if user has queries 5000 records ( for\nexample ), a request
will be made to query notes for all those 5000\nnotes which leads to
performance issue and sometimes error as
shown\nbelow:\n\n\n![image](https://github.com/user-attachments/assets/6fcfe05d-340c-4dcb-a273-5af53ed12945)\n\n\n##
👨‍💻 Changes\n\nThis adds attribute `pageIndex` to timeline state.
\n\n```javascript\n{\n \"pageIndex\": number\n}\n```\n`pageIndex` helps
with getting the events for that particular page.\n\n## 🟡 Caveat\n\n-
Currently this `pageIndex` is shared between Query and EQL tabs
which\ncan lead to wonky behavior at time.\n- Additionally, as of now
table maintains its own page index and\nconsumer component cannot effect
the `pageIndex` of the
UnifiedDataGrid.","sha":"de9d5465df5900936991d79306cb2cbbe63f4623"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201240","number":201240,"mergeCommit":{"message":"[Security
Solution] Adds callback `onUpdatePageIndex` to get current `pageIndex`
in Unified Data table (#201240)\n\n## Summary\n\nHandles resolution
for\n- Notes fetching data for all Timeline Records which leads
to\nperformance issues.\n-
#201330 \n\n## Issue - Notes
fetching data for all Timeline Records \n\nCurrently, there was no way
for consumer of `UnifiedDataGrid` to get the\ncurrent `pageIndex`.
Security Solution needs to get the current\n`pageIndex` so the items on
the current page can be calculated.\n\n@elastic/kibana-data-discovery ,
please let us know if you have any\nopinion here.\n\nThis results in
notes being fetched for all Timeline Records which means\nminimum of 500
records and if user has queries 5000 records ( for\nexample ), a request
will be made to query notes for all those 5000\nnotes which leads to
performance issue and sometimes error as
shown\nbelow:\n\n\n![image](https://github.com/user-attachments/assets/6fcfe05d-340c-4dcb-a273-5af53ed12945)\n\n\n##
👨‍💻 Changes\n\nThis adds attribute `pageIndex` to timeline state.
\n\n```javascript\n{\n \"pageIndex\": number\n}\n```\n`pageIndex` helps
with getting the events for that particular page.\n\n## 🟡 Caveat\n\n-
Currently this `pageIndex` is shared between Query and EQL tabs
which\ncan lead to wonky behavior at time.\n- Additionally, as of now
table maintains its own page index and\nconsumer component cannot effect
the `pageIndex` of the
UnifiedDataGrid.","sha":"de9d5465df5900936991d79306cb2cbbe63f4623"}},{"branch":"8.16","label":"v8.16.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
  • Loading branch information
logeekal authored Dec 2, 2024
1 parent 5fbc576 commit 4309f10
Show file tree
Hide file tree
Showing 17 changed files with 738 additions and 252 deletions.
60 changes: 60 additions & 0 deletions packages/kbn-unified-data-table/src/components/data_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1399,4 +1399,64 @@ describe('UnifiedDataTable', () => {
EXTENDED_JEST_TIMEOUT
);
});

describe('pagination', () => {
const onChangePageMock = jest.fn();
beforeEach(() => {
jest.clearAllMocks();
});
test('should effect pageIndex change', async () => {
const component = await getComponent({
...getProps(),
onUpdatePageIndex: onChangePageMock,
rowsPerPageState: 1,
rowsPerPageOptions: [1, 5],
});

expect(findTestSubject(component, 'pagination-button-1').exists()).toBeTruthy();
onChangePageMock.mockClear();
findTestSubject(component, 'pagination-button-1').simulate('click');
expect(onChangePageMock).toHaveBeenNthCalledWith(1, 1);
});

test('should effect pageIndex change when itemsPerPage has been changed', async () => {
/*
* Use Case:
*
* Let's say we have 4 pages and we are on page 1 with 1 item per page.
* Now if we change items per page to 4, it should automatically change the pageIndex to 0.
*
* */
const component = await getComponent({
...getProps(),
onUpdatePageIndex: onChangePageMock,
rowsPerPageState: 1,
rowsPerPageOptions: [1, 4],
});

expect(findTestSubject(component, 'pagination-button-4').exists()).toBeTruthy();
onChangePageMock.mockClear();
// go to last page
findTestSubject(component, 'pagination-button-4').simulate('click');
expect(onChangePageMock).toHaveBeenNthCalledWith(1, 4);
onChangePageMock.mockClear();

// Change items per Page so that pageIndex autoamtically changes.
expect(findTestSubject(component, 'tablePaginationPopoverButton').text()).toBe(
'Rows per page: 1'
);
findTestSubject(component, 'tablePaginationPopoverButton').simulate('click');
component.setProps({
rowsPerPageState: 5,
});

await waitFor(() => {
expect(findTestSubject(component, 'tablePaginationPopoverButton').text()).toBe(
'Rows per page: 5'
);
});

expect(onChangePageMock).toHaveBeenNthCalledWith(1, 0);
});
});
});
59 changes: 37 additions & 22 deletions packages/kbn-unified-data-table/src/components/data_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,12 @@ export interface UnifiedDataTableProps {
* Update rows per page state
*/
onUpdateRowsPerPage?: (rowsPerPage: number) => void;
/**
*
* this callback is triggered when user navigates to a different page
*
*/
onUpdatePageIndex?: (pageIndex: number) => void;
/**
* Configuration option to limit sample size slider
*/
Expand Down Expand Up @@ -493,6 +499,7 @@ export const UnifiedDataTable = ({
getRowIndicator,
dataGridDensityState,
onUpdateDataGridDensity,
onUpdatePageIndex,
}: UnifiedDataTableProps) => {
const { fieldFormats, toastNotifications, dataViewFieldEditor, uiSettings, storage, data } =
services;
Expand All @@ -519,6 +526,8 @@ export const UnifiedDataTable = ({
docIdsInSelectionOrder,
} = selectedDocsState;

const [currentPageIndex, setCurrentPageIndex] = useState(0);

useEffect(() => {
if (!hasSelectedDocs && isFilterActive) {
setIsFilterActive(false);
Expand Down Expand Up @@ -596,50 +605,56 @@ export const UnifiedDataTable = ({
typeof rowsPerPageState === 'number' && rowsPerPageState > 0
? rowsPerPageState
: DEFAULT_ROWS_PER_PAGE;
const [pagination, setPagination] = useState({
pageIndex: 0,
pageSize: currentPageSize,
});

const rowCount = useMemo(() => (displayedRows ? displayedRows.length : 0), [displayedRows]);
const pageCount = useMemo(
() => Math.ceil(rowCount / pagination.pageSize),
[rowCount, pagination]
() => Math.ceil(rowCount / currentPageSize),
[rowCount, currentPageSize]
);

useEffect(() => {
/**
* Syncs any changes in pageIndex because of changes in pageCount
* to the consumer.
*
*/
setCurrentPageIndex((previousPageIndex: number) => {
const calculatedPageIndex = previousPageIndex > pageCount - 1 ? 0 : previousPageIndex;
if (calculatedPageIndex !== previousPageIndex) {
onUpdatePageIndex?.(calculatedPageIndex);
}
return calculatedPageIndex;
});
}, [onUpdatePageIndex, pageCount]);

const paginationObj = useMemo(() => {
const onChangeItemsPerPage = (pageSize: number) => {
onUpdateRowsPerPage?.(pageSize);
};

const onChangePage = (pageIndex: number) =>
setPagination((paginationData) => ({ ...paginationData, pageIndex }));
const onChangePage = (newPageIndex: number) => {
setCurrentPageIndex(newPageIndex);
onUpdatePageIndex?.(newPageIndex);
};

return isPaginationEnabled
? {
onChangeItemsPerPage,
onChangePage,
pageIndex: pagination.pageIndex > pageCount - 1 ? 0 : pagination.pageIndex,
pageSize: pagination.pageSize,
pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(pagination.pageSize),
pageIndex: currentPageIndex,
pageSize: currentPageSize,
pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(currentPageSize),
}
: undefined;
}, [
isPaginationEnabled,
pagination.pageIndex,
pagination.pageSize,
pageCount,
rowsPerPageOptions,
onUpdateRowsPerPage,
currentPageSize,
currentPageIndex,
onUpdatePageIndex,
]);

useEffect(() => {
setPagination((paginationData) =>
paginationData.pageSize === currentPageSize
? paginationData
: { ...paginationData, pageSize: currentPageSize }
);
}, [currentPageSize, setPagination]);

const unifiedDataTableContextValue = useMemo<DataTableContext>(
() => ({
expanded: expandedDoc,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export type OnColumnRemoved = (columnId: ColumnId) => void;
export type OnColumnResized = ({ columnId, delta }: { columnId: ColumnId; delta: number }) => void;

/** Invoked when a user clicks to load more item */
export type OnChangePage = (nextPage: number) => void;
export type OnFetchMoreRecords = (nextPage: number) => void;

/** Invoked when a user checks/un-checks a row */
export type OnRowSelected = ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { UnifiedTimeline } from '../unified_components';
import { defaultUdtHeaders } from './column_headers/default_headers';
import type { UnifiedTimelineBodyProps } from './unified_timeline_body';
import { UnifiedTimelineBody } from './unified_timeline_body';
import { render, screen } from '@testing-library/react';
import { render } from '@testing-library/react';
import { defaultHeaders, mockTimelineData, TestProviders } from '../../../../common/mock';

jest.mock('../unified_components', () => {
Expand All @@ -32,17 +32,14 @@ const defaultProps: UnifiedTimelineBodyProps = {
isTextBasedQuery: false,
itemsPerPage: 25,
itemsPerPageOptions: [10, 25, 50],
onChangePage: jest.fn(),
onFetchMoreRecords: jest.fn(),
refetch: jest.fn(),
rowRenderers: [],
sort: [],
timelineId: 'timeline-1',
totalCount: 0,
updatedAt: 0,
pageInfo: {
activePage: 0,
querySize: 0,
},
onUpdatePageIndex: jest.fn(),
};

const renderTestComponents = (props?: UnifiedTimelineBodyProps) => {
Expand All @@ -57,39 +54,6 @@ describe('UnifiedTimelineBody', () => {
beforeEach(() => {
(UnifiedTimeline as unknown as jest.Mock).mockImplementation(MockUnifiedTimelineComponent);
});
it('should pass correct page rows', () => {
const { rerender } = renderTestComponents();

expect(screen.getByTestId('unifiedTimelineBody')).toBeVisible();
expect(MockUnifiedTimelineComponent).toHaveBeenCalledTimes(2);

expect(MockUnifiedTimelineComponent).toHaveBeenLastCalledWith(
expect.objectContaining({
events: mockEventsData.flat(),
}),
{}
);

const newEventsData = structuredClone([mockEventsData[0]]);

const newProps = {
...defaultProps,
pageInfo: {
activePage: 1,
querySize: 0,
},
events: newEventsData,
};

MockUnifiedTimelineComponent.mockClear();
rerender(<UnifiedTimelineBody {...newProps} />);
expect(MockUnifiedTimelineComponent).toHaveBeenLastCalledWith(
expect.objectContaining({
events: [...mockEventsData, ...newEventsData].flat(),
}),
{}
);
});

it('should pass default columns when empty column list is supplied', () => {
const newProps = { ...defaultProps, columns: [] };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,20 @@
*/

import type { ComponentProps, ReactElement } from 'react';
import React, { useEffect, useState, useMemo } from 'react';
import React, { useMemo } from 'react';
import { RootDragDropProvider } from '@kbn/dom-drag-drop';
import { StyledTableFlexGroup, StyledUnifiedTableFlexItem } from '../unified_components/styles';
import { UnifiedTimeline } from '../unified_components';
import { defaultUdtHeaders } from './column_headers/default_headers';
import type { PaginationInputPaginated, TimelineItem } from '../../../../../common/search_strategy';

export interface UnifiedTimelineBodyProps extends ComponentProps<typeof UnifiedTimeline> {
header: ReactElement;
pageInfo: Pick<PaginationInputPaginated, 'activePage' | 'querySize'>;
}

export const UnifiedTimelineBody = (props: UnifiedTimelineBodyProps) => {
const {
header,
isSortEnabled,
pageInfo,
columns,
rowRenderers,
timelineId,
Expand All @@ -33,28 +30,14 @@ export const UnifiedTimelineBody = (props: UnifiedTimelineBodyProps) => {
refetch,
dataLoadingState,
totalCount,
onChangePage,
onFetchMoreRecords,
activeTab,
updatedAt,
trailingControlColumns,
leadingControlColumns,
onUpdatePageIndex,
} = props;

const [pageRows, setPageRows] = useState<TimelineItem[][]>([]);

const rows = useMemo(() => pageRows.flat(), [pageRows]);

useEffect(() => {
setPageRows((currentPageRows) => {
if (pageInfo.activePage !== 0 && currentPageRows[pageInfo.activePage]?.length) {
return currentPageRows;
}
const newPageRows = pageInfo.activePage === 0 ? [] : [...currentPageRows];
newPageRows[pageInfo.activePage] = events;
return newPageRows;
});
}, [events, pageInfo.activePage]);

const columnsHeader = useMemo(() => columns ?? defaultUdtHeaders, [columns]);

return (
Expand All @@ -73,16 +56,17 @@ export const UnifiedTimelineBody = (props: UnifiedTimelineBodyProps) => {
itemsPerPage={itemsPerPage}
itemsPerPageOptions={itemsPerPageOptions}
sort={sort}
events={rows}
events={events}
refetch={refetch}
dataLoadingState={dataLoadingState}
totalCount={totalCount}
onChangePage={onChangePage}
onFetchMoreRecords={onFetchMoreRecords}
activeTab={activeTab}
updatedAt={updatedAt}
isTextBasedQuery={false}
trailingControlColumns={trailingControlColumns}
leadingControlColumns={leadingControlColumns}
onUpdatePageIndex={onUpdatePageIndex}
/>
</RootDragDropProvider>
</StyledUnifiedTableFlexItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export type {
OnColumnsSorted,
OnColumnRemoved,
OnColumnResized,
OnChangePage,
OnFetchMoreRecords as OnChangePage,
OnPinEvent,
OnRowSelected,
OnSelectAll,
Expand Down
Loading

0 comments on commit 4309f10

Please sign in to comment.