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

Dashboard breaks on unrestorable session state in URL #71461

Closed
flash1293 opened this issue Jul 13, 2020 · 6 comments · Fixed by #74264
Closed

Dashboard breaks on unrestorable session state in URL #71461

flash1293 opened this issue Jul 13, 2020 · 6 comments · Fixed by #74264
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:StateManagement Feature:Visualizations Generic visualization features (in case no more specific feature label is available) regression Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@flash1293
Copy link
Contributor

flash1293 commented Jul 13, 2020

Kibana version: >= 7.7

Describe the bug: When accessing a dashboard URL having state:storeInSessionStorage enabled, dashboard fails to render if the state hashes in the URL can't be resolved.

Steps to reproduce:

  1. Enable state:storeInSessionStorage
  2. Go to a dashboard
  3. Open a new incognito tab (or tab in another browser) and copy/paste the URL from your main tab
  4. Empty page renders

Expected behavior:
The dashboard should render based on the saved object state and a toast should be shown informing the user the URL state could not the restored.

Any additional context:
Stack trace:

angular.js:15570 Error: Unable to completely restore the URL, be sure to use the share functionality.
    at throwUnableToRestoreUrlError (state_hash.ts:50)
    at retrieveState (state_hash.ts:56)
    at decodeState (encode_decode_state.ts:27)
    at kbn_url_storage.ts:44
    at Array.forEach (<anonymous>)
    at getStatesFromKbnUrl (kbn_url_storage.ts:43)
    at getStateFromKbnUrl (kbn_url_storage.ts:60)
    at Object.get (create_kbn_url_state_storage.ts:62)
    at new DashboardStateManager (dashboard_state_manager.ts:99)
    at new DashboardAppController (dashboard_app_controller.tsx:114) "<div ng-view="" class="dshAppContainer ng-scope">"

This is the line throwing the error:https://github.com/flash1293/kibana/blob/bf04235dae35452061cc7ea3d86d96c19a58206c/src/plugins/dashboard/public/application/dashboard_state_manager.ts#L132

It should be guarded with a try/catch block - if the URL state can't be restored, it should just continue the initialization routine based on the state it already got from the saved object.

As Visualize and Discover are using very similar state handling mechanisms, they are most likely affected by this as well and should be fixed the same way.

@flash1293 flash1293 added bug Fixes for quality problems that affect the customer experience Feature:Dashboard Dashboard related features regression Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jul 13, 2020
@elasticmachine
Copy link
Contributor

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

@inqueue
Copy link
Member

inqueue commented Jul 15, 2020

The same is reproduced when right clicking the Visualize field link from the Discover app to visualize in a new tab.

@flash1293 flash1293 added Feature:Discover Discover Application Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jul 16, 2020
@flash1293
Copy link
Contributor Author

Good catch @inqueue , it's using a very similar technique for state handling. I updated the description.

@flash1293
Copy link
Contributor Author

cc @Dosant - one option would be to handle this error case within the state management helper from utils by just returning undefined and showing the toast, wdyt? Then we could fix all cases of this at once.

@Dosant
Copy link
Contributor

Dosant commented Jul 16, 2020

@flash1293, I agree this should be better handled inside the util, but I'd also prefer to not pass toastService inside of it.

Anyway, because it isn't a plugin, but just a set of stateless functions, will have to pass something everywhere explicitly. So maybe make it more generic at allow to pass onError function?

It also not only about try / catching initial get function. I think if error happens during subsequent syncing, it just crashes now and observables inside state containers error out - so anyway improvements are needed inside the utils

I think we should put generic error handling improvements on app-arch roadmap, but if we want to fix this asap I think at least try / catch initial get would be a good improvement.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant Dosant self-assigned this Jul 30, 2020
Dosant added a commit that referenced this issue Aug 6, 2020
Fixes #71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see #71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in #71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.


Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit to Dosant/kibana that referenced this issue Aug 6, 2020
Fixes elastic#71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see elastic#71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in elastic#71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.


Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit to Dosant/kibana that referenced this issue Aug 6, 2020
Fixes elastic#71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see elastic#71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in elastic#71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/timelion/public/app.js
#	x-pack/plugins/lens/public/app_plugin/app.tsx
Dosant added a commit that referenced this issue Aug 10, 2020
Fixes #71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see #71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in #71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.


Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:StateManagement Feature:Visualizations Generic visualization features (in case no more specific feature label is available) regression Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants