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

[fix] Adding new visualization when session storage enabled #73353

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Jul 27, 2020

Summary

Closes #73013

The above issue was caused by the conflict between browserHistory and hashHistory, where subscribing to hashHistory clears the history.location.state. The following code sandbox illustrates the problem.

When store in session storage is enabled, the location state is cleared prior to the creation of the dashboard container, which resulted in no embeddable being added. Once #65614 is closed, and angular routing is removed from dashboard, the location state should no longer be cleared.
Once we have switched away from using hash routing #67470, this issue should be resolved.

In the meantime, this workaround loads the incomingEmbeddable much earlier in the dashboard_app_controller's constructor.

For maintainers

@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes labels Jul 27, 2020
@ThomThomson ThomThomson requested a review from a team July 27, 2020 20:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Chrome and Firefox, adding and editing Lens and Visualize panels works fine with session storage enabled.

Once #65614 is closed, and angular routing is removed from dashboard, the location state should no longer be cleared.

Are you sure about that? We are using hash history outside of Angular as well for the state syncing, it probably has the same issue of losing the incoming state.

One reason more to switch away from hash routing completely soon in the near future :)

@ThomThomson
Copy link
Contributor Author

A cursory look into lens and the deangularized visualize suggested that they didn't suffer from the same problem, but I've done a bit more digging, and it looks like you're right @flash1293. This shows the problem showing up in visualize:

Screen Shot 2020-07-28 at 1 04 17 PM

Longer term, is the goal to move towards browser history? Is there an issue set up to track this?

If not, I am wondering if we should consider session storage as a more stable alternative for the state transfer service.

@flash1293
Copy link
Contributor

@ThomThomson There is a toplevel issue here: #67470

@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 29, 2020

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/uptime/public/components/common/charts/__tests__.PingHistogram component renders the component without errors

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 8 times on tracked branches: https://github.com/elastic/kibana/issues/73522


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `PingHistogram component renders the component without errors 1`

- Snapshot  - 1
+ Received  + 1

@@ -3,11 +3,11 @@
      class="euiTitle euiTitle--xsmall"
    >
      Pings over time
    </h2>,
    <div
-     aria-label="Bar Chart showing uptime status over time from a year ago to a year ago."
+     aria-label="Bar Chart showing uptime status over time from 2 years ago to 2 years ago."
      style="height:100%;opacity:1;transition:opacity 0.2s"
    >
      <div
        class="echChart"
      >
    at Object.it (/dev/shm/workspace/kibana/x-pack/plugins/uptime/public/components/common/charts/__tests__/ping_histogram.test.tsx:58:23)
    at Promise (/dev/shm/workspace/kibana/node_modules/jest-circus/build/utils.js:198:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/dev/shm/workspace/kibana/node_modules/jest-circus/build/utils.js:162:10)
    at _callCircusTest (/dev/shm/workspace/kibana/node_modules/jest-circus/build/run.js:205:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Build metrics

page load bundle size

id value diff baseline
dashboard 678.9KB +40.0B 678.9KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Cannot add visualization when session storage is on
5 participants