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

[Discuss][Meta] Dashboard By Value URL length considerations #71499

Closed
ThomThomson opened this issue Jul 13, 2020 · 16 comments · Fixed by #86939
Closed

[Discuss][Meta] Dashboard By Value URL length considerations #71499

ThomThomson opened this issue Jul 13, 2020 · 16 comments · Fixed by #86939
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Project:TimeToVisualize RFC Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@ThomThomson
Copy link
Contributor

ThomThomson commented Jul 13, 2020

Related to: #84561

Problem

In the Time to Visualize project, embeddables can be stored by value inside dashboards. When a panel is stored by value, its state consists of all input required by the embeddable type instead of the panel's state consisting only of a saved object ID.

Currently, when a dashboard is in edit mode, the state of every panel is stored in the url, even the much larger by value panels.

To illustrate the problem, here is a quick comparison of edit url length:

Dashboard Name Panel Count / Strategy Character Count
New dashboard 1 panel by reference 575
ECommerce Dashboard 12 panels by reference 2976
New dashboard 1 panel by value 6075
New dashboard 1 map by value ~10000

Doing a rough estimate / extrapolation it seems like a dashboard with 200 complicated by value panels could potentially exceed chrome's 2MB limit.

It's important to stress that crashing chrome like this is an unlikely edge case, therefore I wouldn't consider this a blocker for flipping the allowByValueEmbeddables switch. That said, releasing a project which balloons the url length without a plan to mitigate it doesn't seem like a great idea, not to mention the aesthetics of gigantic urls.

A Solution

Recently, the state of dashboard panels in view mode was moved from the URL to state storage, which significantly cut down on the URL length in view mode.

I would suggest removing panels from the url in edit mode as well. Instead, the current state of the dashboards panels would be stored in sessionStorage, in a new object where the keys are dashboard Ids, and the values are listings of dashboard panels. When a panel is edited, removed or added, the sessionStorage is kept in sync. Saving would replace the panels in the saved object with the panels from session storage, and canceling the edit would do the reverse.

This would allow us to persist unsaved edits to multiple dashboards over reloads and navigations. If we go with this solution, we will have to take into account a number of considerations:

Considerations

Potential for Data Loss

There is a concern that removing the unsaved edits from the url could result in the loss of this data. This concern could be mitigated with the following changes:

  • Storing unsaved panels in session storage would mean that the data would only be lost when the session is ended, i.e. when the browser tab is closed.
  • The OnAppLeave handler will be used to ensure that the user doesn't leave dashboard with unsaved changes [Time to Visualize] Dashboard Warn on App Leave #81360

Snapshot sharing

Currently, users are able to share unsaved edits to a dashboard without using the short url function. This lets users without write privileges share unsaved edits to dashboards. This functionality could be retained with the following implementation details:

  • When loading a dashboard: if panels were provided in the url from a snapshot share, push those panels into the session storage and remove them from the url. If there are panels in the session storage, load the dashboard using those panels, if not use the panels directly from the saved object.
  • When sharing: Select short url by default. When snapshot share is selected and short url is deselected, show a warning that the url can be very long. Pull panels from the sessionStorage to create the full snapshot url as before.

Undo / Redo

Currently, panels in the url provide implicit undo / redo behaviour via the browser back and forward buttons. If they were removed from the url, we would have to provide an alternative means of achieving this such as memory based undo / redo #8887. This is an area with a big potential for improvement, since the current implementation has many problems that can be solved with this approach.

  • Changes to the navigation can get lost because each panel added pushes a new url to the browser history.
  • Pressing back accidentally (via a mouse button or a swipe maybe) can actually remove a panel. If a by value panel is removed like this the user could lose work.
  • It's not obvious that the browser back and forward buttons provide this feature, because this is non-standard

UX Improvements this could unlock

  • It could be implemented first in the dashboard_state_manager then standardized for use in other applications.
  • We could potentially hook up this undo redo system to the shortcuts ctrl + z & ctrl + y which are industry standard
  • We could provide a ui in the new dashboard toolbar that includes undo & redo buttons.

Open Questions

  • If we implement in memory undo / redo, it would be a separate initiative from Time to Visualize? I would hope so.
  • If that is the case, would it be acceptable if there was a minor in which undo / redo on dashboards was not possible?
@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:AppArch RFC labels Jul 13, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@ThomThomson ThomThomson changed the title [RFC] Dashboard with By Value Embeddables URL length considerations [Discuss] Dashboard with By Value Embeddables URL length considerations Jul 13, 2020
@flash1293
Copy link
Contributor

flash1293 commented Jul 14, 2020

I don't have an opinion yet on this, but a few things to take into consideration:

Currently the behavior is also inconsistent because of this change, see #66875

Another con to consider is potential data loss - currently the unsaved dashboard state is part of the url, so navigating away from dashboard and back restores all unsaved changes. If these changes would only be kept in the dashboard state manager, they would be lost in that scenario. Lens (and Maps) are showing a modal when the user tries to navigate away from a page without having everything saved - that might be an option for Dashboard as well.

In every way I would consider this a breaking change because you can't use dashboards in the same way as before anymore - we have a lot of users, there are definitely some which would be affected by this change.

Another option we could consider: Saving "by reference" panels in the url, but keeping "by value" panels in the local state only (with a modal when trying to navigate away). "The old way" of doing things wouldn't be affected, and with the next major version we could switch the behavior for "by reference" panels over as well. The thing I'm not sure about is whether this effort is even necessary.

@timroes
Copy link
Contributor

timroes commented Jul 14, 2020

Also don't have an opinion yet, but more considerations:

We're currently relying on the URL handling to achieve undo when editing dashboards (and in other apps). This behavior is really trained by our users, and I am not sure if we can simply drop this behavior. If we would, we def need an alternative undo/redo solution in place.

I also share the concern about losing state, that Joe mentioned, a lot. I think losing a dashboard state that's currently in edit mode, while doing a browser refresh is a no-go, even with warning it (or at least a significant breaking change). Maybe we should at least default to storing the state in localStorage.

Related to the discussion in #35812

@ThomThomson
Copy link
Contributor Author

In every way I would consider this a breaking change because you can't use dashboards in the same way as before anymore - we have a lot of users, there are definitely some which would be affected by this change.

Totally agreed. See @stacey-gammon's comment about potentially removing url state in 8.0.0. Though I think it's also important to discuss what can be done to move us forward before then.

Maybe a combination of both ideas could work in 7.x, where by reference panels show up in the url as before and any edits to by value panels are stored in localStorage, based on the dashboardId -> embeddableId. In the dashboard's getInput method in edit mode, we could load in changes. After saving, the local storage for that dashboard could be cleared.

This method would prevent data loss on refresh/navigation but would still break the ability to share edits to some embeddables on a dashboard via the url which could be frustratingly inconsistent.

@timroes
Copy link
Contributor

timroes commented Jul 15, 2020

Maybe a combination of both ideas could work in 7.x, where by reference panels show up in the url as before and any edits to by value panels are stored in localStorage, based on the dashboardId -> embeddableId. In the dashboard's getInput method in edit mode, we could load in changes. After saving, the local storage for that dashboard could be cleared.

From my understanding this would leave the user with a very weird editing experience? Imagine a user would add a by value panel, then add a by reference panel, then add another by value panel, then resize the by value panel. If they now hit "back" in their browser, wouldn't suddenly just the reference panel be vanishing, and the two by value panels (added before and after it) staying around (in a potentially even broken grid configuration now, if the reference panel was the one holding those two together)?

@flash1293
Copy link
Contributor

If I understood @ThomThomson correctly we should just use the existing URL state hashing functionality only for certain sub properties (the "by value" panels), while encoding everything else directly in the URL as it's working now.

  • (+) Undo/redo works as expected
  • (+) No lost state as long as staying in the session
  • (+) No overly long URLs
  • (-) "sharing unsaved edits to a dashboard without write privileges" still broken as soon as "by value" panels are used
  • (-) potentially confusing behavior when sharing dashboard URLs with "by value" panels (we have a bug around that anyway at the moment: Dashboard breaks on unrestorable session state in URL #71461)

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Jul 15, 2020

What if we implemented "by value" embeddables with a feature flag and as part of that feature flag, removed all panel state from the URL? As long as users had the ability to the old URL state support, we should be able to switch it on by default in 7.x, with a warning that the having the flag off is deprecated and starting in 8.0, won't be supported anymore.

Then we won't be in a situation of some state in URL, some not, but rather "new url state + by value support" or "old url state + no by value support".

@ThomThomson
Copy link
Contributor Author

What if we implemented "by value" embeddables with a feature flag and as part of that feature flag, removed all panel state from the URL?

This strikes me as the best way forward. I am not sure about enabling the flag by default in 7.x, due to breaking changes concerns, but bundling makes the most sense. The two features Visualizations by value and fully descriptive urls are fundamentally at odds, and trying to mix them would definitely result in unnecessary complexity for the user, even if we avoid the type of situation that Tim described.

@stacey-gammon
Copy link
Contributor

My concern with not flipping it on by default is that you wouldn't get anyone trying out the "vis by value" feature, but we can always make that decision at a later date.

@AlonaNadler
Copy link

AlonaNadler commented Jul 16, 2020

Is the ability to share unsaved edits a key functionality of dashboard?

occasionally users need to share the exact state they are looking at, that can be after they narrow the timerange and added a few filters. Will that still be possible?

I got more than several complaints on the existing long URL, mainly since some systems don't allow users to paste the URL once it is above certain characters (zoom, confluent, few others).
In addition, the long URL makes is very inelegant to share with others. The option for a short URL is only available for users with write privilege which dashboard consumers often don't have.

I agree with @timroes comment

I also share the concern about losing state, that Joe mentioned, a lot. I think losing a dashboard state that's currently in edit mode, while doing a browser refresh is a no-go, even with warning it (or at least a significant breaking change). Maybe we should at least default to storing the state in localStorage.

Artificial way to reduce the likelihood of sharing a long URL : currently when clicking on share the default is a snapshot which is the unsaved version of the dashboard. In general, I think it should be the object (often people are can't figure why they don't see the dashboard latest updates) it will also help ensure the default sharing is using short URL. Again it is not a solution, more like an improvement

Based on past experience with flags, users are not changing the default and it is not providing us any feedback to add new capability if the fly is off by default.

Regarding breaking change, it is breaking change if the existing URLs users have doesn't work anymore or start showing something else

@linyaru
Copy link

linyaru commented Jul 17, 2020

@ThomThomson asked me to share some thoughts as an end user.

Regarding using backward/forward nav as undo/redo buttons: I have never used it for this purpose, and always thought it was an UI oversight. I am on my laptop a lot while working dashboards and accidentally navigate away from my current version all the time (because of tracking pad movements) and it is super inconvenient; instead I try to save my dashboard periodically, or whenever I remember.

Regarding verbose URL: we have a specific use case where we dynamically generate links to a dashboard based on different filter values; the verbose URL makes it harder to generate the URL, and also some systems don't allow us to embed URLs this long.

Occasionally we save dashboards and close them, and then trying to re-open them by using browser auto-complete will bring up an older unsaved version of the dashboards; we'd have to go back to the Dashboard tab and try to find the object again; this is inconvenient to say the least.

@ThomThomson
Copy link
Contributor Author

Note from today's meeting:

  1. Ensure that moving panels from URL to state storage doesn't break backwards compatibility of previously created dashboard share links from edit mode or view mode. In order to do this
    possible solition? if panels are present in the url, the dashboard app should load those panels into the state storage instead of the panels from the saved object.

  2. Ensure that it is still possible to share unsaved dashboard states.
    possible solution? we could leverage the short urls service and include the panel state taken from state storage.

  3. Ensure that unsaved dashboard state is not lost when navigating between apps.
    possible solution? The state storage contains a registry of 'unsaved panel states' by dashboard id, which is read from on each load. This way the user can have multiple unsaved edits at the same time, and can navigate freely without losing their state. Saving or canceling will clear the dashboard id in that registry.

@cjcenizal
Copy link
Contributor

cjcenizal commented Dec 8, 2020

We're currently relying on the URL handling to achieve undo when editing dashboards (and in other apps). This behavior is really trained by our users, and I am not sure if we can simply drop this behavior. If we would, we def need an alternative undo/redo solution in place.

I just want to note that this enhancement is tracked by #8887, though it's ancient and maybe needs to be updated with the latest concerns if this is something we want to move forward with.

I also share the concern about losing state, that Joe mentioned, a lot. I think losing a dashboard state that's currently in edit mode, while doing a browser refresh is a no-go, even with warning it (or at least a significant breaking change). Maybe we should at least default to storing the state in localStorage.

Losing form state by refreshing or navigating away from the form is a looming concern for ES UI. We manage many forms, some of which can be very complex, and losing state this way is painful. I'd be strongly in favor of a generalized solution to this problem that tracks form state in local storage and attempts to automatically repopulate it if the user navigates back and then forward again in browser history, refreshes the browser, or navigates away and then hits the back button.

This tutorial demonstrates one method of solving this problem leveraging History.state. Not sure if this is the path we should take but I wanted to share it to show that it might be a simpler problem that it seems. Note that browsers have different size limits for History.state, the lowest being Firefox at 640 kb per this Stack Overflow.

@flash1293
Copy link
Contributor

Another possible approach (not saying it's perfect, but it's an option) is to prevent the user from navigating away in a way they would lose data. Maps and Lens are using this at the moment:
Screenshot 2020-12-09 at 09 09 11

@ThomThomson
Copy link
Contributor Author

@flash1293 Good call, I am working on the onAppLeave Handler implementation for dashboard as we speak. I do think this can be a temporary solution to the problem of losing unsaved state that @cjcenizal has brought up.

Even with this approach though, I think the URL length considerations are still valid due to the fact each by value panel contains so much information that a dashboard with 200 by value panels could potentially exceed chrome's 2MB limit. Even though that is quite unlikely, I also consider the aesthetics of having such massive urls a deal breaker.

If we move forward with removing panel state from the url, we will have to consider the user moving back and forth between apps while they edit. This necessitates a place to store the edited state while the user is gone. My current idea for the implementation is as follows:

  • Populating Session Storage When entering edit mode, copy the panels into session storage saved with the dashboard id as a key. This way unsaved edits will persist across reloads, and navigation, but will only exist in the current tab. Because of the dashboard id as a key, users can have unsaved edits in multiple dashboards.
  • On Dashboard Load if panels were provided in the url (from share perhaps), push those panels into the session storage and remove them from the url. If there are panels in the session storage, load the dashboard using those panels, if not use the panels directly from the saved object.
  • Clearing From Session Storage On cancel or save, the panels are cleared from session storage.
  • On Dashboard Share change the default share to the 'saved object' share, allow snapshot share but add a warning that it may generate very long urls. As a followup, a more explicit share filters and time range checkbox should be added because the difference between 'saved object' share and 'snapshot' share on a dashboard in view mode is unclear.

The only feature this implementation is missing is undo-redo, but it could be added later via an in-memory solution as per #8887

Please let me know if you see any glaring issues in this implementation plan

@ThomThomson ThomThomson changed the title [Discuss] Dashboard with By Value Embeddables URL length considerations [Discuss][Meta] Dashboard By Value URL length considerations Dec 15, 2020
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 Project:TimeToVisualize RFC Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants