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

[Visualize] Make sure session is updated and passed to the embeddable #123538

Merged
merged 4 commits into from
Jan 27, 2022

Conversation

flash1293
Copy link
Contributor

Fixes #121791

Updating the session from the top nav would not trigger a reload because the input of the embeddable handler (which is used in the editor as well) is not updated. This was also the case for time range changes - the vis refreshed in these cases because the time range changed, but it wouldn't start a new session for this so everything would go into the same session. This shouldn't happen - on every change in filters/query/timerange a new session is required.

This PR changes the following:

  • In the on refresh handler, only a new session is started
  • the current session is passed to the embeddable in reloadVisualization so it is recognizing something changed even if it's just the session
  • Start a new session every time the query state changes
  • If the session changes and it's not updated yet (would happen if we come from reloadVisualization), update the embeddable handler input

This has to be reviewed carefully from someone knowing their way around visualize.

@flash1293 flash1293 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure backport:skip This commit does not require backporting v8.1.0 labels Jan 21, 2022
@flash1293 flash1293 marked this pull request as ready for review January 21, 2022 17:07
@flash1293 flash1293 requested a review from a team as a code owner January 21, 2022 17:07
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM, Refresh works fine now. I did many tests and I can't find any regression.
@alexwizp do you want to also take a look?

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 enabled auto-merge (squash) January 27, 2022 09:26
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
visualizations 139.1KB 139.3KB +177.0B

History

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

@flash1293 flash1293 merged commit b5a47fd into elastic:main Jan 27, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with Refresh button
5 participants