Skip to content
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

[Lens][Agg based] Fix table pagination bug for both lens and agg based #139160

Merged
merged 5 commits into from
Aug 23, 2022

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Aug 19, 2022

Fixes #139126

There were two problems working together to cause this issue:

  • Several places in the code don't check whether a row actually exists before accessing it - this crashes the visualization.
  • The Lens table did not reset the page if the data changes to make the current page out of bounds (this worked fine for the agg based table already)

This PR is doing two things:

  • Making sure to check whether a row exists before trying access data
  • Adding the logic from agg based table to Lens table to reset the page to the last feasible page if the number of rows shrinks due to an update.

The second point can't happen on filter click because the number of rows can shrink due to a large number of different causes (basically everything that triggers a new data load, e.g. hitting refresh, auto-refresh, changing time range, changing the query, ...)

Kapture.2022-08-19.at.11.26.30.mp4

@flash1293 flash1293 added Feature:Data Table Data table visualization feature release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Aug 19, 2022
@flash1293 flash1293 marked this pull request as ready for review August 19, 2022 11:16
@flash1293 flash1293 requested a review from a team as a code owner August 19, 2022 11:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested—works great.

@flash1293
Copy link
Contributor Author

It seems like we are just a few hours too late for 8.4.0, let’s wait until early next week to merge it and release it with 8.4.1

@drewdaemon
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #21 / ObservabilityApp Observability alerts / Add to case When user has read permissions for cases does not render case options in the overflow menu

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.2MB 1.2MB +305.0B
visTypeTable 15.7KB 15.9KB +133.0B
total +438.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Aug 24, 2022
@drewdaemon drewdaemon added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v8.3.0 labels Aug 24, 2022
kibanamachine pushed a commit that referenced this pull request Aug 24, 2022
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.1 Backport failed because of merge conflicts
8.3

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 139160

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

kibanamachine added a commit that referenced this pull request Aug 24, 2022
…g based (#139160) (#139393)

* [Lens][Agg based] Fix table pagination bug for both lens and agg based (#139160)

(cherry picked from commit 90f2c80)

* resolve type issue

Co-authored-by: Joe Reuter <[email protected]>
Co-authored-by: Andrew Tate <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Data Table Data table visualization feature Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.4 v8.4.1 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filtering from second page of lens and agg based table breaks the table viz.
5 participants