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

Preserve variables and time range when navigating between dashboards #87966

Merged
merged 21 commits into from
May 27, 2024

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented May 16, 2024

Available under preserveDashboardStateWhenNavigating feature toggle.

This is a POC of a functionality that will allow preserving currently selected filters and group by dimensions dashboard variables and time range when navigating between different dashboards. It's an early WIP to gather some opinions.

How it works?

  1. Currently selected variable and time range are captured on DashboardScene deactivation and stored in the local storage under a tab-specific key.
  2. When new dashboard scene is loaded, we read from local storage and replace the location in the browser with the saved query params, so that when the URL sync is activated, the variables will restore preserved values.
  3. When one navigates away from Grafana the preserved filters are cleared.

TODO:

  • Deduplicate query params with the same value to avoid duplicate values in scenarios when someone navigates back and forts between the same dashboard (using browser's back button).
  • On the target dashboard only apply those query params that match variable names - thi I'm not sure if we need this as this is supposed to work as part of the newDashboardWithFiltersAndGroupBy that guarantees the same name of the filters and group by variable across different dashboards.
  • Preserve filters with tab id, so that closing one tab will not cause clearing the filters for other tabs. Or for other tabs not to override filters stored by other tabs.
  • Add test coverage

@dprokop dprokop marked this pull request as ready for review May 20, 2024 12:42
@dprokop dprokop requested review from grafanabot and a team as code owners May 20, 2024 12:42
@dprokop dprokop requested review from ivanortegaalba and removed request for a team May 20, 2024 12:42
@dprokop
Copy link
Member Author

dprokop commented May 20, 2024

@torkelo @ashharrison90 - i still need to implement tests, but this should be ready for approach review and testing. Available under preserveDashboardStateWhenNavigating feature toggle.

@dprokop dprokop changed the title POC preserve filters and group by when navigating between dashboards Preserve variables and time range when navigating between dashboards May 20, 2024
@dprokop dprokop added the no-changelog Skip including change in changelog/release notes label May 20, 2024
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good! some minor refactoring that could simplify and reduce the string/url manipulation, and missing tests but otherwise this looks really good.

An interesting thing to discuss with @thanos-karachalios and @Ijin08 would be how users could opt in to this when moving between specific dashboards (ctrl+click or something, or maybe an icon in the dashboard search or dashboard links)

if (!scene.state.uid) {
return;
}
const variables = scene.state.$variables?.state.variables;
Copy link
Member

Choose a reason for hiding this comment

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

any reason not using UrlSyncManager.getUrlState(scene) here? (I know it might have more state you don't care about but we can filter that out either before save or when we try to use it)

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

behaviour-wise looks great 🙌

just a couple of structure suggestions that i think will help make this cleaner

public/app/app.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

great job on this 👏👏👏

just some minor nits

@dprokop dprokop requested a review from torkelo May 22, 2024 14:06
const variables = scene.state.$variables?.state.variables;
const timeRange = scene.state.$timeRange;

let urlStates: UrlQueryMap = variables
Copy link
Member

Choose a reason for hiding this comment

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

Still not sure why we are not using UrlSyncManager.getUrlState(scene) here

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it's better to opt-in for params to sync, rather than removing items from the full state (may include setting, panel edit, view panel id etc...).

Copy link
Member

Choose a reason for hiding this comment

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

@dprokop but instead of removing state you don't care about just include from & to and all that start with var- , that is basically the same as the code here but much smaller and less complex

Copy link
Member Author

Choose a reason for hiding this comment

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

just pushed 👍

@dprokop dprokop merged commit 6e9543e into main May 27, 2024
18 checks passed
@dprokop dprokop deleted the filters-preserve-when-navigating branch May 27, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/frontend no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants