Fix row rendering in infinite scroll #8060

Swiddis · 2024-09-06T19:24:15Z

Description

When rendering infinite scrolling PPL/SQL in the new Discover UI, the rendering was stalling and entering an infinite loading loop. The root cause turns out to be that there's a small amount of fade-out at the bottom of the OSD application:

This was causing an issue with the progress bar observer responsible for loading the next batch of rows: it had threshold: 1.0 set which meant that it would only trigger if the progress bar is 100% visible. This isn't a problem for DQL where large amounts of data records have a limit disclaimer at the bottom:

But when that disclaimer isn't present (for PPL and SQL), this fade effect would cover the progress bar, causing it not to meet the 1.0 threshold:

In addition to fixing the bug, this PR adds a batch of unit tests for the discover table.

Issues Resolved

N/A

Screenshot

After reaching the end of the available records:

Testing the changes

The PR features a new set of unit tests for the involved functionality, but given the nature of the bug it's hard to test the fault directly (we would need to mock the entire OSD application and assert that the intersection threshold is > 0.5 when we scroll to the end). Theoretically possible, but probably too much effort for a one-line fix.

Changelog

fix: Fix row rendering in Discover infinite scroll

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: Simeon Widdis <[email protected]>

github-actions · 2024-09-06T19:24:48Z

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 8060.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

codecov · 2024-09-06T19:40:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.83%. Comparing base (22f1d46) to head (2e815cb).
Report is 62 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8060      +/-   ##
==========================================
+ Coverage   63.71%   63.83%   +0.12%     
==========================================
  Files        3738     3738              
  Lines       88682    88684       +2     
  Branches    13788    13789       +1     
==========================================
+ Hits        56501    56614     +113     
+ Misses      31593    31476     -117     
- Partials      588      594       +6

Flag	Coverage Δ
Linux_1	`29.96% <33.33%> (+<0.01%)`	⬆️
Linux_2	`58.79% <ø> (ø)`
Linux_3	`40.25% <100.00%> (+0.19%)`	⬆️
Linux_4	`31.20% <ø> (ø)`
Windows_1	`29.97% <33.33%> (+<0.01%)`	⬆️
Windows_2	`58.74% <ø> (ø)`
Windows_3	`40.25% <100.00%> (+0.19%)`	⬆️
Windows_4	`31.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx

Signed-off-by: Simeon Widdis <[email protected]>

github-actions · 2024-09-06T21:34:00Z

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

Signed-off-by: Simeon Widdis <[email protected]>

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx

Signed-off-by: Simeon Widdis <[email protected]>

ashwin-pc · 2024-09-10T21:45:37Z

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx

+            <EuiProgress
+              size="xs"
+              color="accent"
+              data-test-subj="discoverRenderedRowsProgress"
+              style={{
+                // Add a little margin if we aren't rendering the truncation callout below, to make
+                // the progress bar render better when it's not present
+                marginBottom: rows.length !== sampleSize ? '5px' : '0',
+              }}
+            />


Why arent we adding the truncation banner for PPL and SQL?

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.

ashwin-pc

Thanks Simeon!

sejli

LGTM, I reran the build and test workflow that failed. I don't think it's related to your changes, but will wait for that before merging first.

Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit fbac07b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

(cherry picked from commit fbac07b) Signed-off-by: Simeon Widdis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

Swiddis added 3 commits September 5, 2024 16:27

Add data-test-subj to discover rendered rows progress bar

0d0ff91

Signed-off-by: Simeon Widdis <[email protected]>

Add some explanation for the renderer

80e85c8

Signed-off-by: Simeon Widdis <[email protected]>

Fix desired row updates in row loading for infinite scroll

ac69252

Signed-off-by: Simeon Widdis <[email protected]>

github-actions bot added the repeat-contributor label Sep 6, 2024

github-actions bot added the failed changeset label Sep 6, 2024

kavilla reviewed Sep 6, 2024

View reviewed changes

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx Outdated Show resolved Hide resolved

kavilla reviewed Sep 6, 2024

View reviewed changes

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx Outdated Show resolved Hide resolved

Swiddis added 3 commits September 6, 2024 14:04

Add unit tests

631d496

Signed-off-by: Simeon Widdis <[email protected]>

Add test that rendering resumes on new data

9391c20

Signed-off-by: Simeon Widdis <[email protected]>

Update test sample data threshold to go beyond lookahead limit

111eb79

Signed-off-by: Simeon Widdis <[email protected]>

Changeset file for PR opensearch-project#8060 created/updated

5023ae4

github-actions bot removed the failed changeset label Sep 6, 2024

kavilla added backport 2.x discover for discover reinvent v2.17.0 backport 2.17 discover-next labels Sep 6, 2024

Update fix for new diagnosis

344f641

Signed-off-by: Simeon Widdis <[email protected]>

AMoo-Miki reviewed Sep 6, 2024

View reviewed changes

...ins/discover/public/application/components/default_discover_table/default_discover_table.tsx Outdated Show resolved Hide resolved

Swiddis added 3 commits September 6, 2024 16:14

Add trailing margin for progress bar if callout not rendered

0863c51

Signed-off-by: Simeon Widdis <[email protected]>

Lower threshold further

32a795b

Signed-off-by: Simeon Widdis <[email protected]>

Merge branch 'main' into discover-progress-bugfix

03823cd

Swiddis marked this pull request as ready for review September 6, 2024 23:17

Swiddis requested review from ananzh, ashwin-pc, joshuarrrr and abbyhu2000 as code owners September 6, 2024 23:17

Swiddis requested review from bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric, LDrago27, virajsanghvi, sejli, joshuali925 and huyaboo as code owners September 6, 2024 23:17

Swiddis requested review from AMoo-Miki and kavilla September 9, 2024 18:01

Swiddis added 3 commits September 9, 2024 16:24

Merge branch 'main' into discover-progress-bugfix

367a0f8

Remove extraneous scrolling effect variable

39b1440

Signed-off-by: Simeon Widdis <[email protected]>

Merge branch 'main' into discover-progress-bugfix

2e815cb

ashwin-pc reviewed Sep 10, 2024

View reviewed changes

ashwin-pc approved these changes Sep 10, 2024

View reviewed changes

sejli approved these changes Sep 10, 2024

View reviewed changes

Swiddis mentioned this pull request Sep 10, 2024

[BUG] No truncation callout for Discover when query default record count isn't exactly 500 #8114

Open

joshuali925 approved these changes Sep 10, 2024

View reviewed changes

joshuali925 merged commit fbac07b into opensearch-project:main Sep 10, 2024
66 of 67 checks passed

opensearch-trigger-bot bot mentioned this pull request Sep 10, 2024

[Backport 2.x] Fix row rendering in infinite scroll #8115

Merged

opensearch-trigger-bot bot mentioned this pull request Sep 10, 2024

[Backport 2.17] Fix row rendering in infinite scroll #8116

Merged

AMoo-Miki added the in-next label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix row rendering in infinite scroll #8060

Fix row rendering in infinite scroll #8060

Swiddis commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024

codecov bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024

ashwin-pc Sep 10, 2024

Swiddis Sep 10, 2024

ashwin-pc left a comment

sejli left a comment

Fix row rendering in infinite scroll #8060

Fix row rendering in infinite scroll #8060

Conversation

Swiddis commented Sep 6, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Sep 6, 2024

ℹ️ Manual Changeset Creation Reminder

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 6, 2024

❌ Invalid Prefix For Manual Changeset Creation

ashwin-pc Sep 10, 2024

Choose a reason for hiding this comment

Swiddis Sep 10, 2024

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

sejli left a comment

Choose a reason for hiding this comment

Swiddis commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading