-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix row rendering in infinite scroll #8060
Merged
joshuali925
merged 14 commits into
opensearch-project:main
from
Swiddis:discover-progress-bugfix
Sep 10, 2024
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0d0ff91
Add data-test-subj to discover rendered rows progress bar
Swiddis 80e85c8
Add some explanation for the renderer
Swiddis ac69252
Fix desired row updates in row loading for infinite scroll
Swiddis 631d496
Add unit tests
Swiddis 9391c20
Add test that rendering resumes on new data
Swiddis 111eb79
Update test sample data threshold to go beyond lookahead limit
Swiddis 5023ae4
Changeset file for PR #8060 created/updated
opensearch-changeset-bot[bot] 344f641
Update fix for new diagnosis
Swiddis 0863c51
Add trailing margin for progress bar if callout not rendered
Swiddis 32a795b
Lower threshold further
Swiddis 03823cd
Merge branch 'main' into discover-progress-bugfix
Swiddis 367a0f8
Merge branch 'main' into discover-progress-bugfix
Swiddis 39b1440
Remove extraneous scrolling effect variable
Swiddis 2e815cb
Merge branch 'main' into discover-progress-bugfix
Swiddis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
fix: | ||
- Fix row rendering in Discover infinite scroll ([#8060](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8060)) |
158 changes: 158 additions & 0 deletions
158
...over/public/application/components/default_discover_table/default_discover_table.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
/* | ||
* 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 100; | ||
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(100).keys()].map((key) => { | ||
return { | ||
_id: key.toString(), | ||
fields: { | ||
textField: `value${key}`, | ||
longField: key, | ||
'@timestamp': new Date((1720000000 + key) * 1000), | ||
}, | ||
}; | ||
}); | ||
|
||
const getDefaultDiscoverTable = (hitsOverride?: OpenSearchSearchHit[]) => ( | ||
<IntlProvider locale="en"> | ||
<DefaultDiscoverTable | ||
columns={['textField', 'longField', '@timestamp']} | ||
rows={(hitsOverride ?? hits) as OpenSearchSearchHit[]} | ||
indexPattern={indexPattern} | ||
sort={[]} | ||
onSort={jest.fn()} | ||
onRemoveColumn={jest.fn()} | ||
onMoveColumn={jest.fn()} | ||
onAddColumn={jest.fn()} | ||
onFilter={jest.fn()} | ||
/> | ||
</IntlProvider> | ||
); | ||
|
||
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 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(); | ||
}); | ||
}); | ||
|
||
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"]'); | ||
|
||
// Keep scrolling until all the current rows are exhausted | ||
while (sentinel) { | ||
const mockScrollEntry = { isIntersecting: true, target: sentinel }; | ||
act(() => { | ||
intersectionObserverCallback([mockScrollEntry] as IntersectionObserverEntry[]); | ||
}); | ||
sentinel = container.querySelector('div[data-test-subj="discoverRenderedRowsProgress"]'); | ||
} | ||
|
||
// Make the other rows available | ||
rerender(getDefaultDiscoverTable(hits as OpenSearchSearchHit[])); | ||
|
||
await waitFor(() => { | ||
const progressSentinel = container.querySelector( | ||
'div[data-test-subj="discoverRenderedRowsProgress"]' | ||
); | ||
expect(progressSentinel).toBeInTheDocument(); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why arent we adding the truncation banner for PPL and SQL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require being able to get the active selected language in-context since PPL and SQL have a different default number of records they return when they have no limit configured, I briefly tried to follow the stack trace to see if I could pass that context down but couldn't find where it exists. Figured fixing the core bug was more important than the bells & whistles for this PR, could look at it for a future PR.