From 0d0ff91cdade4817048adc084310eeb599dd1ccc Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 5 Sep 2024 16:26:04 -0700 Subject: [PATCH 01/11] Add data-test-subj to discover rendered rows progress bar Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index ae7e9632fbd7..654c3a693a7d 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -254,7 +254,7 @@ const DefaultDiscoverTableUI = ({ {!showPagination && renderedRowCount < rows.length && (
- +
)} {!showPagination && rows.length === sampleSize && ( From 80e85c8905ffb6ded592883282d3a2377ded662a Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 11:48:24 -0700 Subject: [PATCH 02/11] Add some explanation for the renderer Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index 654c3a693a7d..ac90e37d6e41 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -5,7 +5,7 @@ import './_doc_table.scss'; -import React, { useEffect, useMemo, useRef, useState, useCallback } from 'react'; +import React, { useEffect, useMemo, useRef, useState, useCallback, SetStateAction } from 'react'; import { EuiSmallButtonEmpty, EuiCallOut, EuiProgress } from '@elastic/eui'; import { FormattedMessage } from '@osd/i18n/react'; import { TableHeader } from './table_header'; @@ -155,6 +155,10 @@ const DefaultDiscoverTableUI = ({ const lazyLoadRequestFrameRef = useRef(0); const lazyLoadLastTimeRef = useRef(0); + // When doing infinite scrolling, the `rows` prop gets regularly updated from the outside: we only + // render the additional rows when we know the load isn't too high. To prevent overloading the + // renderer, we throttle by current framerate and only render if the frames are fast enough, then + // we increase the rendered row count and trigger a re-render. React.useEffect(() => { if (!showPagination) { const loadMoreRows = (time: number) => { From ac69252eb07fb1ac8d4b995012ddab32a703f18f Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 12:20:30 -0700 Subject: [PATCH 03/11] Fix desired row updates in row loading for infinite scroll Signed-off-by: Simeon Widdis --- .../default_discover_table.tsx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index ac90e37d6e41..3145671402b1 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -41,6 +41,8 @@ export interface DefaultDiscoverTableProps { // ToDo: These would need to be read from an upcoming config panel const PAGINATED_PAGE_SIZE = 50; const INFINITE_SCROLLED_PAGE_SIZE = 10; +// How far to queue unrendered rows ahead of time during infinite scrolling +const DESIRED_ROWS_LOOKAHEAD = 5 * INFINITE_SCROLLED_PAGE_SIZE; const DefaultDiscoverTableUI = ({ columns, @@ -86,7 +88,7 @@ const DefaultDiscoverTableUI = ({ */ const [renderedRowCount, setRenderedRowCount] = useState(INFINITE_SCROLLED_PAGE_SIZE); const [desiredRowCount, setDesiredRowCount] = useState( - Math.min(rows.length, 5 * INFINITE_SCROLLED_PAGE_SIZE) + Math.min(rows.length, DESIRED_ROWS_LOOKAHEAD) ); const [displayedRows, setDisplayedRows] = useState(rows.slice(0, PAGINATED_PAGE_SIZE)); const [currentRowCounts, setCurrentRowCounts] = useState({ @@ -118,7 +120,7 @@ const DefaultDiscoverTableUI = ({ if (entries[0].isIntersecting) { // Load another batch of rows, some immediately and some lazily setRenderedRowCount((prevRowCount) => prevRowCount + INFINITE_SCROLLED_PAGE_SIZE); - setDesiredRowCount((prevRowCount) => prevRowCount + 5 * INFINITE_SCROLLED_PAGE_SIZE); + setDesiredRowCount((prevRowCount) => prevRowCount + DESIRED_ROWS_LOOKAHEAD); } }, { threshold: 1.0 } @@ -170,12 +172,17 @@ const DefaultDiscoverTableUI = ({ lazyLoadLastTimeRef.current = time; lazyLoadRequestFrameRef.current = requestAnimationFrame(loadMoreRows); } + // Ensure we have more desired rows in the queue to prevent stalling when we render the + // current desired row count + if (renderedRowCount + DESIRED_ROWS_LOOKAHEAD > desiredRowCount) { + setDesiredRowCount(Math.min(renderedRowCount + DESIRED_ROWS_LOOKAHEAD, rows.length)); + } }; lazyLoadRequestFrameRef.current = requestAnimationFrame(loadMoreRows); } return () => cancelAnimationFrame(lazyLoadRequestFrameRef.current); - }, [showPagination, renderedRowCount, desiredRowCount]); + }, [showPagination, renderedRowCount, desiredRowCount, rows.length]); // Allow auto column-sizing using the initially rendered rows and then convert to fixed const tableLayoutRequestFrameRef = useRef(0); From 631d496dc3029d63d3462face3d09e48dcc13853 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 14:04:48 -0700 Subject: [PATCH 04/11] Add unit tests Signed-off-by: Simeon Widdis --- .../default_discover_table.test.tsx | 151 ++++++++++++++++++ .../default_discover_table.tsx | 2 +- 2 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx new file mode 100644 index 000000000000..b2f373a7225b --- /dev/null +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx @@ -0,0 +1,151 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import React from 'react'; +import { IntlProvider } from 'react-intl'; +import { act, render, waitFor, screen } from '@testing-library/react'; +import { DefaultDiscoverTable } from './default_discover_table'; +import { OpenSearchSearchHit } from '../../doc_views/doc_views_types'; +import { coreMock } from '../../../../../../core/public/mocks'; +import { getStubIndexPattern } from '../../../../../data/public/test_utils'; + +jest.mock('../../../opensearch_dashboards_services', () => ({ + getServices: jest.fn().mockReturnValue({ + uiSettings: { + get: jest.fn().mockImplementation((key) => { + switch (key) { + case 'discover:sampleSize': + return 50; + case 'shortDots:enable': + return true; + case 'doc_table:hideTimeColumn': + return false; + case 'discover:sort:defaultOrder': + return 'desc'; + default: + return null; + } + }), + }, + }), +})); + +describe('DefaultDiscoverTable', () => { + const indexPattern = getStubIndexPattern( + 'test-index-pattern', + (cfg) => cfg, + '@timestamp', + [ + { name: 'textField', type: 'text' }, + { name: 'longField', type: 'long' }, + { name: '@timestamp', type: 'date' }, + ], + coreMock.createSetup() + ); + + // Generate 50 hits with sample fields + const hits = [...Array(50).keys()].map((key) => { + return { + _id: key.toString(), + fields: { + textField: `value${key}`, + longField: key, + '@timestamp': new Date((1720000000 + key) * 1000), + }, + }; + }); + + const getDefaultDiscoverTable = () => ( + + + + ); + + let intersectionObserverCallback: (entries: IntersectionObserverEntry[]) => void = (_) => {}; + const mockIntersectionObserver = jest.fn(); + + beforeEach(() => { + mockIntersectionObserver.mockImplementation((...args) => { + intersectionObserverCallback = args[0]; + return { + observe: () => null, + unobserve: () => null, + disconnect: () => null, + }; + }); + window.IntersectionObserver = mockIntersectionObserver; + }); + + it('should render the correct number of rows initially', () => { + const { container } = render(getDefaultDiscoverTable()); + + const tableRows = container.querySelectorAll('tbody tr'); + expect(tableRows.length).toBe(10); + }); + + it('should load more rows when scrolling to the bottom', async () => { + const { container } = render(getDefaultDiscoverTable()); + + const sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); + const mockScrollEntry = { isIntersecting: true, target: sentinel }; + act(() => { + intersectionObserverCallback([mockScrollEntry] as IntersectionObserverEntry[]); + }); + + await waitFor(() => { + const tableRows = container.querySelectorAll('tbody tr'); + expect(tableRows.length).toBe(20); + }); + }); + + it('should not load more rows than the total number of rows', async () => { + const { container } = render(getDefaultDiscoverTable()); + + const sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); + + // Simulate scrolling to the bottom multiple times + for (let i = 0; i < 3; i++) { + const mockScrollEntry = { isIntersecting: true, target: sentinel }; + act(() => { + intersectionObserverCallback([mockScrollEntry] as IntersectionObserverEntry[]); + }); + } + + await waitFor(() => { + const tableRows = container.querySelectorAll('tbody tr'); + expect(tableRows.length).toBe(40); + }); + }); + + it('should display the sample size callout when all rows are rendered', async () => { + const { container } = render(getDefaultDiscoverTable()); + + let sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); + + // Simulate scrolling to the bottom until all rows are rendered + while (sentinel) { + const mockScrollEntry = { isIntersecting: true, target: sentinel }; + act(() => { + intersectionObserverCallback([mockScrollEntry] as IntersectionObserverEntry[]); + }); + sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); + } + + await waitFor(() => { + const callout = screen.getByTestId('discoverDocTableFooter'); + expect(callout).toBeInTheDocument(); + }); + }); +}); diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index 3145671402b1..a0d8f54274f9 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -5,7 +5,7 @@ import './_doc_table.scss'; -import React, { useEffect, useMemo, useRef, useState, useCallback, SetStateAction } from 'react'; +import React, { useEffect, useMemo, useRef, useState, useCallback } from 'react'; import { EuiSmallButtonEmpty, EuiCallOut, EuiProgress } from '@elastic/eui'; import { FormattedMessage } from '@osd/i18n/react'; import { TableHeader } from './table_header'; From 9391c20f1c8ff5f983421a6cb23eea882e25ce7a Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 14:22:24 -0700 Subject: [PATCH 05/11] Add test that rendering resumes on new data Signed-off-by: Simeon Widdis --- .../default_discover_table.test.tsx | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx index b2f373a7225b..967ea2d7750c 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx @@ -57,11 +57,11 @@ describe('DefaultDiscoverTable', () => { }; }); - const getDefaultDiscoverTable = () => ( + const getDefaultDiscoverTable = (hitsOverride?: OpenSearchSearchHit[]) => ( { }); }); - it('should not load more rows than the total number of rows', async () => { + it('should display the sample size callout when all rows are rendered', async () => { const { container } = render(getDefaultDiscoverTable()); - const sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); + let sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); - // Simulate scrolling to the bottom multiple times - for (let i = 0; i < 3; i++) { + // Simulate scrolling to the bottom until all rows are rendered + while (sentinel) { const mockScrollEntry = { isIntersecting: true, target: sentinel }; act(() => { intersectionObserverCallback([mockScrollEntry] as IntersectionObserverEntry[]); }); + sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); } await waitFor(() => { - const tableRows = container.querySelectorAll('tbody tr'); - expect(tableRows.length).toBe(40); + const callout = screen.getByTestId('discoverDocTableFooter'); + expect(callout).toBeInTheDocument(); }); }); - it('should display the sample size callout when all rows are rendered', async () => { - const { container } = render(getDefaultDiscoverTable()); + it('Should restart rendering when new data is available', async () => { + const truncHits = hits.slice(0, 35) as OpenSearchSearchHit[]; + const { container, rerender } = render(getDefaultDiscoverTable(truncHits)); let sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); - // Simulate scrolling to the bottom until all rows are rendered + // Keep scrolling until all the current rows are exhausted while (sentinel) { const mockScrollEntry = { isIntersecting: true, target: sentinel }; act(() => { @@ -143,9 +145,14 @@ describe('DefaultDiscoverTable', () => { sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); } + // Make the other rows available + rerender(getDefaultDiscoverTable(hits as OpenSearchSearchHit[])); + await waitFor(() => { - const callout = screen.getByTestId('discoverDocTableFooter'); - expect(callout).toBeInTheDocument(); + const progressSentinel = container.querySelector( + 'div[data-test-subj="discoverRenderedRowsProgress"]' + ); + expect(progressSentinel).toBeInTheDocument(); }); }); }); From 111eb790196192d66a62e3cb29313c12cecc6494 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 14:28:21 -0700 Subject: [PATCH 06/11] Update test sample data threshold to go beyond lookahead limit Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx index 967ea2d7750c..2cff704bbf67 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.test.tsx @@ -17,7 +17,7 @@ jest.mock('../../../opensearch_dashboards_services', () => ({ get: jest.fn().mockImplementation((key) => { switch (key) { case 'discover:sampleSize': - return 50; + return 100; case 'shortDots:enable': return true; case 'doc_table:hideTimeColumn': @@ -46,7 +46,7 @@ describe('DefaultDiscoverTable', () => { ); // Generate 50 hits with sample fields - const hits = [...Array(50).keys()].map((key) => { + const hits = [...Array(100).keys()].map((key) => { return { _id: key.toString(), fields: { From 5023ae46c20d232539357a50323f08438cd5dec4 Mon Sep 17 00:00:00 2001 From: "opensearch-changeset-bot[bot]" <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Date: Fri, 6 Sep 2024 21:44:26 +0000 Subject: [PATCH 07/11] Changeset file for PR #8060 created/updated --- changelogs/fragments/8060.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/8060.yml diff --git a/changelogs/fragments/8060.yml b/changelogs/fragments/8060.yml new file mode 100644 index 000000000000..705e428a42a1 --- /dev/null +++ b/changelogs/fragments/8060.yml @@ -0,0 +1,2 @@ +fix: +- Fix row rendering in Discover infinite scroll ([#8060](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8060)) \ No newline at end of file From 344f64145c6ac254c96976279aace39be15dd6a6 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 15:48:06 -0700 Subject: [PATCH 08/11] Update fix for new diagnosis Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index a0d8f54274f9..58fe53d0f64f 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -123,7 +123,11 @@ const DefaultDiscoverTableUI = ({ setDesiredRowCount((prevRowCount) => prevRowCount + DESIRED_ROWS_LOOKAHEAD); } }, - { threshold: 1.0 } + { + // Important that 0 < threshold < 1, since there OSD application div has a transparent + // fade at the bottom which causes the sentinel element to sometimes not be 100% visible + threshold: 0.5, + } ); observerRef.current.observe(sentinelElement); @@ -172,11 +176,6 @@ const DefaultDiscoverTableUI = ({ lazyLoadLastTimeRef.current = time; lazyLoadRequestFrameRef.current = requestAnimationFrame(loadMoreRows); } - // Ensure we have more desired rows in the queue to prevent stalling when we render the - // current desired row count - if (renderedRowCount + DESIRED_ROWS_LOOKAHEAD > desiredRowCount) { - setDesiredRowCount(Math.min(renderedRowCount + DESIRED_ROWS_LOOKAHEAD, rows.length)); - } }; lazyLoadRequestFrameRef.current = requestAnimationFrame(loadMoreRows); } From 0863c51a25a3f828c042ad116190def413947351 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 16:14:18 -0700 Subject: [PATCH 09/11] Add trailing margin for progress bar if callout not rendered Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index 58fe53d0f64f..91817fb00fab 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -264,7 +264,16 @@ const DefaultDiscoverTableUI = ({ {!showPagination && renderedRowCount < rows.length && (
- +
)} {!showPagination && rows.length === sampleSize && ( From 32a795b530f71b0147d942d6a7f64bd64e2d39d9 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 6 Sep 2024 16:15:37 -0700 Subject: [PATCH 10/11] Lower threshold further Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index 91817fb00fab..2268de60180f 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -126,7 +126,7 @@ const DefaultDiscoverTableUI = ({ { // Important that 0 < threshold < 1, since there OSD application div has a transparent // fade at the bottom which causes the sentinel element to sometimes not be 100% visible - threshold: 0.5, + threshold: 0.1, } ); From 39b14400420b87bdf442bdb69524126886187d93 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 10 Sep 2024 09:48:04 -0700 Subject: [PATCH 11/11] Remove extraneous scrolling effect variable Signed-off-by: Simeon Widdis --- .../default_discover_table/default_discover_table.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx index 2268de60180f..1e92858157bc 100644 --- a/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx +++ b/src/plugins/discover/public/application/components/default_discover_table/default_discover_table.tsx @@ -181,7 +181,7 @@ const DefaultDiscoverTableUI = ({ } return () => cancelAnimationFrame(lazyLoadRequestFrameRef.current); - }, [showPagination, renderedRowCount, desiredRowCount, rows.length]); + }, [showPagination, renderedRowCount, desiredRowCount]); // Allow auto column-sizing using the initially rendered rows and then convert to fixed const tableLayoutRequestFrameRef = useRef(0);